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

@@ -514,6 +514,7 @@ class Test extends AsyncResource {
  }

  async run() {
+


please revert unrelated change

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

lib/internal/test_runner/tests_stream.js

@@ -27,6 +27,10 @@ class TestsStream extends Readable {
    }
  }

+  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

@@ -27,6 +27,10 @@ class TestsStream extends Readable {
    }
  }

+  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

@@ -27,6 +27,10 @@ class TestsStream extends Readable {
    }
  }

+  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

@@ -29,6 +29,10 @@ class TestsStream extends Readable {
    }
  }

+  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

@@ -97,7 +97,7 @@ describe('node:test reporters', { concurrency: true }, () => {
                               testFile]);
      assert.strictEqual(child.stderr.toString(), '');
      const stdout = 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

@@ -189,7 +189,7 @@ class Test extends AsyncResource {
      this.nesting = 0;
      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');


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

@@ -782,6 +782,8 @@ class Suite extends Test {
  }

  async run() {
+


please revert

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

lib/internal/test_runner/test.js

@@ -478,7 +478,7 @@ class Test extends AsyncResource {
      this.parent.addPendingSubtest(deferred);
      return deferred.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

@@ -29,6 +29,10 @@ class TestsStream extends Readable {
    }
  }

+  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

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

lib/internal/test_runner/test.js

@@ -782,6 +782,8 @@ class Suite extends Test {

}

async run() {

Copy link

Member

MoLow May 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: emit start event when subtest starts #47797

test_runner: emit start event when subtest starts #47797

sankalp1999 commented May 1, 2023

nodejs-github-bot commented May 1, 2023

sankalp1999 commented May 9, 2023

sankalp1999 commented May 9, 2023 •

edited

Loading

MoLow May 10, 2023

MoLow May 10, 2023

sankalp1999 May 10, 2023

cjihrig May 10, 2023

cjihrig commented May 24, 2023

sankalp1999 commented May 25, 2023 •

edited

Loading

sankalp1999 May 25, 2023

sankalp1999 commented May 25, 2023

MoLow May 28, 2023

MoLow May 28, 2023

MoLow commented May 28, 2023

sankalp1999 May 29, 2023

sankalp1999 commented May 30, 2023

MoLow May 30, 2023 •

edited

Loading

MoLow May 31, 2023

MoLow May 31, 2023

cjihrig May 31, 2023

MoLow May 31, 2023

		@@ -514,6 +514,7 @@ class Test extends AsyncResource {
		}

		async run() {

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

		@@ -782,6 +782,8 @@ class Suite extends Test {
		}

		async run() {

test_runner: emit start event when subtest starts #47797

test_runner: emit start event when subtest starts #47797

Conversation

sankalp1999 commented May 1, 2023

nodejs-github-bot commented May 1, 2023

sankalp1999 commented May 9, 2023

sankalp1999 commented May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented May 24, 2023

sankalp1999 commented May 25, 2023 • edited Loading

Choose a reason for hiding this comment

sankalp1999 commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow commented May 28, 2023

Choose a reason for hiding this comment

sankalp1999 commented May 30, 2023

MoLow May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sankalp1999 commented May 9, 2023 •

edited

Loading

sankalp1999 commented May 25, 2023 •

edited

Loading

MoLow May 30, 2023 •

edited

Loading