test_runner: emit start event when subtest starts by sankalp1999 · Pull Request #47797 · nodejs/node · GitHub

emit a start event to show message just after a test/subtest start

Review requested:

@nodejs/test_runner

@MoLow I request your review.

Thanks @MoLow. I have made a test:begin (name_to_be_decided) event in the test_streams file since test:start is already being used.

Not sure if I need to make changes in the textual output in the reporters as you mentioned "this means the event will only be usable through the run API but not via one of the existing reporters"

@MoLow
(#46727 (comment))

If code changes look fine, can proceed with doc changes and tests (a file pointer would be helpful in this case)

please revert unrelated change

does anything call this method?
I would start by adding this method to this test file, then make sure it passes

node/test/parallel/test-runner-reporters.js

Line 93 in 1b17793

    
           assert.match(stdout, /{"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);

no this is the new event so, not called anywhere yet.

Just throwing an idea out there: having both begin and start is confusing. I could also picture people asking for more of these lifecycle events. I would namespace this event further and name it test:lifecycle:run, where lifecycle can be used to indicate what is happening in the test runner and is less about reporting results.

@sankalp1999 are you still working on this?

@cjihrig apologise, got busy with other stuff. Getting back to try this next.

can remove ...directive I guess

Ok I saw upstream has changed significantly.

emit a start event to show message just after a test/subtest start Fixes: nodejs#46727

This commit implements the start event in tests_stream.js and removes the code that was altering TAP output in test.js

Suggested change

      
              begin(nesting, file, testNumber, name, details) {
          
              lifecycleRun(nesting, file, testNumber, name, details) {

Suggested change

      
                  assert.match(stdout, /{"test:lifecycle:run":\d+,"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);
          
                  assert.match(stdout, /{"test:lifecycle:run":4,"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);

there should be a deterministic number of tests running

there seem to be related test failures

will remove comments in next commit if CI passes.

CI was passing. Removed the comments.

I think I can proceed with the doc changes as well.

please revert

I think this should be emitted before the if

I think test:enqueue is a better name, @cjihrig WDYT?

I don't feel too strongly, but I wouldn't say "enqueue" just rolls off the tongue.

I agree, but in terms of distinction from test:start I think it is much better than test:lifecycle:run

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels May 1, 2023

sankalp1999 marked this pull request as draft May 1, 2023 10:46

targos removed the dont-land-on-v14.x label May 3, 2023

sankalp1999 marked this pull request as ready for review May 9, 2023 17:21

MoLow reviewed May 10, 2023

View reviewed changes

cjihrig reviewed May 10, 2023

View reviewed changes

sankalp1999 commented May 25, 2023

View reviewed changes

sankalp1999 added 6 commits May 25, 2023 22:15

test_runner: emit start event when subtest starts

a24f544

emit a start event to show message just after a test/subtest start Fixes: nodejs#46727

test_runner: implement start event in tests_stream.js

7158d9d

This commit implements the start event in tests_stream.js and removes the code that was altering TAP output in test.js

test_runner: revert the first commit

a500692

test_runner: add lifecycle:run call and make tests pass

b2e8085

test_runner:add test:lifecycle:run event at run()

f4e13b5

test_runner: rebase and refactor

d4b38f6

sankalp1999 force-pushed the test-runner-start-event branch from ee93ebc to d4b38f6 Compare May 25, 2023 17:08

MoLow reviewed May 28, 2023

View reviewed changes

test_runner: move method to start(), fix tests

9033855

sankalp1999 commented May 29, 2023

View reviewed changes

test_runner: remove comments and extra line

f7dbe8a

sankalp1999 force-pushed the test-runner-start-event branch from 85a46a4 to f7dbe8a Compare May 30, 2023 09:21

MoLow reviewed May 30, 2023

View reviewed changes

MoLow reviewed May 31, 2023

View reviewed changes

MoLow mentioned this pull request Jun 11, 2023

test_runner: add enqueue and dequeue events #48428

Merged

sankalp1999 closed this Jun 11, 2023

                     this.only = testOnlyFlag;
                     this.reporter = new TestsStream();
-                    this.reporter.begin(this.nesting, kFilename, this.testNumber, this.name, 'starting', 'starting');
+                    // this.reporter.lifecycleRun(this.nesting, kFilename, this.testNumber, this.name, 'starting', 'starting');

	begin(nesting, file, testNumber, name, details) {
	lifecycleRun(nesting, file, testNumber, name, details) {

	assert.match(stdout, /{"test:lifecycle:run":\d+,"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);
	assert.match(stdout, /{"test:lifecycle:run":4,"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);

		}

		async run() {

test_runner: emit start event when subtest starts by sankalp1999 · Pull Request #47797 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: emit start event when subtest starts - #47797

test_runner: emit start event when subtest starts#47797sankalp1999 wants to merge 8 commits intonodejs:mainnodejs/node:mainfrom sankalp1999:test-runner-start-eventsankalp1999/node:test-runner-start-eventCopy head branch name to clipboard

Conversation

sankalp1999 commented May 1, 2023

Uh oh!

nodejs-github-bot commented May 1, 2023

Uh oh!

sankalp1999 commented May 9, 2023

Uh oh!

sankalp1999 commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 24, 2023

Uh oh!

sankalp1999 commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sankalp1999 commented May 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLow commented May 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sankalp1999 commented May 30, 2023

Uh oh!

MoLow May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test_runner: emit start event when subtest starts#47797
sankalp1999 wants to merge 8 commits into
nodejs:mainfrom
sankalp1999:test-runner-start-event

sankalp1999 commented May 9, 2023 •
edited
Loading

sankalp1999 commented May 25, 2023 •
edited
Loading

MoLow May 30, 2023 •
edited
Loading