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.

Sign up for GitHub

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

Jump to bottom

test_runner: fix typescript coverage #49406

Merged
merged 1 commit into from
Sep 4, 2023
Merged

test_runner: fix typescript coverage #49406

merged 1 commit into from
Sep 4, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Aug 30, 2023 •
edited
Loading

Fixes: #49398

not sure how to test this, any help appreciated

@MoLow MoLow requested a review from cjihrig August 30, 2023 05:23
@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 Aug 30, 2023
MoLow
MoLow commented Aug 30, 2023
View reviewed changes
lib/internal/test_runner/test.js Outdated
@@ -737,6 +737,7 @@ class Test extends AsyncResource {
this.reported = true;
reporter.plan(nesting, loc, harness.counters.topLevel);

const coverage = harness.coverage(); // Call this before printing diagnostics, since failure to collect coverage is a diagnostic.
Copy link
Member Author

@MoLow MoLow Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@cjihrig do we have a way to reproduce an error in coverage collection? I want to snapshot this diagnostic that was missing

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the easiest thing would be to monkey patch TestCoverage.prototype.summary() or TestCoverage.prototype.cleanup() so that an error is reported.

Copy link
Member

Choose a reason for hiding this comment

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

We can also create a dumb loader like ts-node that instead of reading typescript file it will read plain txt and execute them as JS

@MoLow MoLow force-pushed the test-runner-typescript-coverage branch from 2a9c0ed to 53924b2 Compare August 30, 2023 05:31
@GeoffreyBooth
Copy link
Member

I think the easiest thing would be to monkey patch TestCoverage.prototype.summary() or TestCoverage.prototype.cleanup() so that an error is reported.

Not sure if this is relevant but in general we’re trying to avoid encouraging users to ever monkey-patch anything; that’s why the Loaders API / module customization hooks exist, and we plan to extend that model to other systems like FS and REPL. We’ve already added import { register } from 'node:module', the plan is to create import { register } from 'node:fs' and from node:repl and so on. Maybe you might want to create import { register } from 'node:test' to allow users to define customization hooks for various parts of the test runner flow?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 30, 2023

we’re trying to avoid encouraging users to ever monkey-patch anything

Unless I misunderstood the original question, @MoLow is trying to trigger an error for the purposes of a Node unit test. This is not something end users should ever be doing.

@GeoffreyBooth
Copy link
Member

This is not something end users should ever be doing.

Excellent, that’s why I wasn’t sure if my comment was relevant. Still though, if/when we need to provide customization abilities for the test runner, like a way to customize output or something, we should consider trying to provide APIs that are somewhat standardized across systems if possible. Maybe that won’t ever be necessary for the test runner since the reporters themselves are so customizable, but I just wanted to bring it up before people started designing new things.

@MoLow
Copy link
Member Author

MoLow commented Aug 31, 2023

@cjihrig you understood me correctly

@MoLow MoLow force-pushed the test-runner-typescript-coverage branch from 53924b2 to 3ed2c63 Compare September 4, 2023 10:07
@MoLow
Copy link
Member Author

MoLow commented Sep 4, 2023

@nodejs/test_runner I believe this is ready for reviews

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53720/

@MoLow MoLow force-pushed the test-runner-typescript-coverage branch from 3ed2c63 to 8564f65 Compare September 4, 2023 15:50
@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53723/

@MoLow MoLow force-pushed the test-runner-typescript-coverage branch from 8564f65 to 92edcf7 Compare September 4, 2023 16:37
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53726/

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 47c5152 into nodejs:main Sep 4, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 47c5152

@nicoabie
Copy link

nicoabie commented Sep 4, 2023

Thanks a lot guys!

This was referenced Sep 5, 2023
Open
Open
Open
Open
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@MoLow @ruyadorno
test_runner: fix typescript coverage
e7285d4 
PR-URL: #49406
Fixes: #49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Sep 28, 2023
Closed
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@MoLow @alexfernandez
test_runner: fix typescript coverage
4a70e26 
PR-URL: nodejs#49406
Fixes: nodejs#49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2023
@MoLow @targos
test_runner: fix typescript coverage
429846f 
PR-URL: #49406
Fixes: #49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: fix typescript coverage
4327a7e 
PR-URL: nodejs/node#49406
Fixes: nodejs/node#49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: fix typescript coverage
e0c4795 
PR-URL: nodejs/node#49406
Fixes: nodejs/node#49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MoLow MoLow deleted the test-runner-typescript-coverage branch May 24, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rluvaton rluvaton rluvaton left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Experimental coverage skips .ts files
7 participants
@MoLow @nodejs-github-bot @GeoffreyBooth @cjihrig @nicoabie @rluvaton @atlowChemi