Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: wait for stream finish when --test-force-exit #54953

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

Fixes #54327.

This PR stores the reporter streams inside of TestsStream, and waits for them to finish before exitting the process.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Sep 15, 2024
@@ -18,6 +18,7 @@ class TestsStream extends Readable {
objectMode: true,
highWaterMark: NumberMAX_SAFE_INTEGER,
});
this.streams = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing new public API that I don't think we want to support.

Copy link
Member Author

@RedYetiDev RedYetiDev Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can redo it with a symbol / _? Unless you know a better way to wait for the streams?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, it doesn't need to be attached to the stream at all. For example, the reporterScope object is internal and could hold them (you would need to make sure it works with watch mode though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO attaching to the stream makes the most sense, it's the.. well... stream.

test/fixtures/test-runner/output/test-runner-force-exit.js Outdated Show resolved Hide resolved
test/parallel/test-runner-output.mjs Outdated Show resolved Hide resolved
test/parallel/test-runner-output.mjs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.07%. Comparing base (7014e50) to head (2e11df6).
Report is 22 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #54953   +/-   ##
=======================================
  Coverage   88.06%   88.07%           
=======================================
  Files         652      652           
  Lines      183545   183558   +13     
  Branches    35854    35859    +5     
=======================================
+ Hits       161648   161670   +22     
+ Misses      15146    15143    -3     
+ Partials     6751     6745    -6     
Files with missing lines Coverage Δ
lib/internal/test_runner/test.js 96.93% <100.00%> (-0.07%) ⬇️
lib/internal/test_runner/tests_stream.js 91.30% <100.00%> (+0.22%) ⬆️
lib/internal/test_runner/utils.js 64.05% <100.00%> (+0.20%) ⬆️

... and 25 files with indirect coverage changes

@RedYetiDev RedYetiDev force-pushed the forceexit-stream branch 4 times, most recently from 339ef9c to 3158d98 Compare September 15, 2024 17:37
@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Sep 17, 2024
@RedYetiDev RedYetiDev marked this pull request as draft September 17, 2024 14:11
@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Sep 17, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review September 17, 2024 15:49
compose(rootReporter, reporter).pipe(destination);
const stream = compose(rootReporter, reporter);
stream.pipe(destination);
ArrayPrototypePush(rootReporter[kReporterStreams], destination);
Copy link
Member Author

@RedYetiDev RedYetiDev Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig the issue was that I was originally appending stream, not destination.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 17, 2024

It appears now it's waiting too long 😅 .

It looks like it's waiting for the schedule to complete, I wonder why? Maybe they are ref'd (edit: if I .unref() it, this no longer accurs).

@cjihrig
Copy link
Contributor

cjihrig commented Sep 18, 2024

Maybe they are ref'd (edit: if I .unref() it, this no longer accurs).

What exactly is ref'ed that you unref'ed? Unfortunately, --test-force-exit really only exists for people that refuse to clean up after their code.

A few things to look into:

  • We might not need to do anything for destinations that are stdout/stderr.
  • For file destinations, the test runner owns those. I don't think we actually close() those streams anywhere though. We probably should for this use case.
  • When creating file streams, we should probably use the flush option.

@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test runner with junit reporter and forceExit flag results in incomplete report
4 participants