test_runner: emit start event when subtest starts#47797

sankalp1999 · 2023-05-01T10:44:17Z

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

Fixes: #46727

nodejs-github-bot · 2023-05-01T10:44:21Z

Review requested:

@nodejs/test_runner

sankalp1999 · 2023-05-09T17:08:34Z

@MoLow I request your review.

sankalp1999 · 2023-05-09T17:11:25Z

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)

MoLow · 2023-05-10T07:11:55Z

lib/internal/test_runner/test.js

 }

 asyncrun(){
+


please revert unrelated change

MoLow · 2023-05-10T07:13:55Z

lib/internal/test_runner/tests_stream.js

 }
 }

+begin(nesting,file,testNumber,name,details,directive){


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.

cjihrig · 2023-05-10T13:36:19Z

lib/internal/test_runner/tests_stream.js

 }

+begin(nesting,file,testNumber,name,details,directive){
+this.#emit('test:begin',{__proto__: null, name, nesting, file, testNumber, details, ...directive});


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.

cjihrig · 2023-05-24T21:30:22Z

@sankalp1999 are you still working on this?

sankalp1999 · 2023-05-25T13:49:18Z

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

sankalp1999 · 2023-05-25T15:01:47Z

lib/internal/test_runner/tests_stream.js


+begin(nesting,file,testNumber,name,details,directive){
+this.#emit('test:lifecycle:run',{__proto__: null, name, nesting, file, testNumber, details, ...directive});
+}


can remove ...directive I guess

sankalp1999 · 2023-05-25T16:44:52Z

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

MoLow · 2023-05-28T07:27:14Z

lib/internal/test_runner/tests_stream.js

 }
 }

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


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

MoLow · 2023-05-28T07:27:58Z

test/parallel/test-runner-reporters.js

 assert.strictEqual(child.stderr.toString(),'');
 conststdout=child.stdout.toString();
-assert.match(stdout,/{"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);
+assert.match(stdout,/{"test:lifecycle:run":\d+,"test:start":4,"test:pass":2,"test:fail":2,"test:plan":2,"test:diagnostic":\d+}$/);


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

MoLow · 2023-05-28T07:29:31Z

there seem to be related test failures

sankalp1999 · 2023-05-29T15:44:33Z

lib/internal/test_runner/test.js

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


will remove comments in next commit if CI passes.

sankalp1999 · 2023-05-30T09:19:40Z

CI was passing. Removed the comments.

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

MoLow · 2023-05-30T16:00:29Z

lib/internal/test_runner/test.js

 }

 asyncrun(){
+


please revert

MoLow · 2023-05-31T00:09:13Z

lib/internal/test_runner/test.js

 returndeferred.promise;
 }
-
+this.reporter.lifecycleRun(this.nesting,kFilename,this.testNumber,this.name,'starting','starting');


I think this should be emitted before the if

MoLow · 2023-05-31T00:09:32Z

lib/internal/test_runner/tests_stream.js

 }

+lifecycleRun(nesting,file,testNumber,name,details){
+this[kEmitMessage]('test:lifecycle:run',{__proto__: null, name, nesting, file, testNumber, details });


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

lib/internal/test_runner/test.js Outdated
}

asyncrun(){

Copy link
Member
MoLowMay 10, 2023
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert unrelated change
sankalp1999 reacted with thumbs up emoji

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 d4b38f6Compare 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 f7dbe8aCompare May 30, 2023 09:21

MoLow reviewed May 30, 2023
View reviewed changes

lib/internal/test_runner/test.js
}

asyncrun(){

Copy link
Member
MoLowMay 30, 2023•
edited
Loading
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert

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

	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+}$/);

Uh oh!

test_runner: emit start event when subtest starts#47797

test_runner: emit start event when subtest starts #47797

Uh oh!

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!

MoLowMay 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

sankalp1999 commented May 9, 2023•
edited
Loading

sankalp1999 commented May 25, 2023•
edited
Loading

MoLowMay 30, 2023•
edited
Loading