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: improve lib/internal/test_runner/test.js coverage #42745

Merged

Conversation

fossamagna
Copy link
Contributor

This improves a test coverage in lib/internal/test_runner/test.js coveage

Ref: https://coverage.nodejs.org/coverage-24adba675179ebba/lib/internal/test_runner/test.js.html#L371

This validates that printing correctly error message when multiple subtests failed

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 15, 2022
@aduh95 aduh95 requested a review from cjihrig April 15, 2022 12:34
@aduh95 aduh95 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 Apr 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but this has conflicts.

@fossamagna
Copy link
Contributor Author

@cjihrig Thanks, I resolved conflicts.

duration_ms: *
failureType: 'testCodeFailure'
error: 'thrown from subtest sync throw fails at first'
code: ERR_TEST_FAILURE
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think the code fields here and below need to be in quotes now. The CI is probably going to fail because of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Oops, I overlooked it. I changed to wrapping error code in single quotes. Thanks again.

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 16, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Apr 16, 2022

@fossamagna can you please rebase on top of master? The merge commit is breaking the CI. Let me know if you help for that.

@lpinca
Copy link
Member

lpinca commented Apr 16, 2022

There is a typo in the first commit message: "coveage" -> coverage".

@fossamagna
Copy link
Contributor Author

@aduh95 Sorry. I will rebase top of master in test-test-runner-coverage branch. Or Should I remake Pull Request in other branch?

@fossamagna fossamagna changed the title test: improve lib/internal/test_runner/test.js coveage test: improve lib/internal/test_runner/test.js coverage Apr 17, 2022
@lpinca
Copy link
Member

lpinca commented Apr 17, 2022

@fossamagna you can force push on this PR branch.

@fossamagna
Copy link
Contributor Author

@lpinca Thanks, I rebase and force push.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8cbc390 into nodejs:master Apr 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8cbc390

@fossamagna fossamagna deleted the test-test-runner-coverage branch April 28, 2022 00:27
targos pushed a commit that referenced this pull request Apr 28, 2022
PR-URL: #42745
Refs: https://coverage.nodejs.org/coverage-24adba675179ebba/lib/internal/test_runner/test.js.html#L371
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 2, 2022
@juanarbol
Copy link
Member

Sadly, this is part of the built-in test-runner, which is not landed in v16.x

aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 8, 2022
aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 9, 2022
aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 9, 2022
PR-URL: nodejs/node#42745
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42745
Refs: https://coverage.nodejs.org/coverage-24adba675179ebba/lib/internal/test_runner/test.js.html#L371
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants