FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: fix typescript coverage by MoLow · Pull Request #49406 · nodejs/node · GitHub

/ node Public

test_runner: fix typescript coverage - #49406

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:test-runner-typescript-coverage
Sep 4, 2023
ConversationCommits (1)ChecksFiles changed
Merged

test_runner: fix typescript coverage#49406
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:test-runner-typescript-coverage

Conversation

MoLow commented Aug 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #49398

not sure how to test this, any help appreciated

MoLow requested a review from cjihrig August 30, 2023 05:23

nodejs-github-bot commented Aug 30, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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 commented Aug 30, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated

MoLow Aug 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

cjihrig Aug 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

rluvaton Aug 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the test-runner-typescript-coverage branch from 2a9c0ed to 53924b2 Compare August 30, 2023 05:31

GeoffreyBooth commented Aug 30, 2023

Copy link
Copy Markdown
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 commented Aug 30, 2023

Copy link
Copy Markdown
Contributor

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 commented Aug 30, 2023

Copy link
Copy Markdown
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 commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

@cjihrig you understood me correctly

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

MoLow commented Sep 4, 2023

Copy link
Copy Markdown
Member Author

@nodejs/test_runner I believe this is ready for reviews

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
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 commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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

cjihrig approved these changes Sep 4, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/output/coverage_failure.js Outdated
MoLow requested a review from atlowChemi September 4, 2023 15:50
MoLow force-pushed the test-runner-typescript-coverage branch from 3ed2c63 to 8564f65 Compare September 4, 2023 15:50
MoLow added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 4, 2023
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 commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow force-pushed the test-runner-typescript-coverage branch from 8564f65 to 92edcf7 Compare September 4, 2023 16:37
MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2023
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 commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2023
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 merged commit 47c5152 into nodejs:main Sep 4, 2023

nodejs-github-bot commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

Landed in 47c5152

nicoabie commented Sep 4, 2023

Copy link
Copy Markdown

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
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
targos pushed a commit that referenced this pull request Nov 27, 2023
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 mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
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 deleted the test-runner-typescript-coverage branch May 24, 2024 09:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rluvaton rluvaton left review comments
cjihrig cjihrig approved these changes
atlowChemi atlowChemi approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL