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: convert new tests to use error types #18581

Closed
wants to merge 1 commit into from
Closed

test: convert new tests to use error types #18581

wants to merge 1 commit into from

Conversation

jackhorton
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Some new tests were added recently that rely on error messages that are incompatible with Node-ChakraCore. This PR simply reverts the test to only relying on error type, rather than error message. vcbuild test is actually failing for me on an unrelated test, sequential/test-inspector-port-cluster, but I might make a new issue for that separately.

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Feb 5, 2018
jasnell
jasnell reviewed Feb 5, 2018
View reviewed changes
test/parallel/test-console-assign-undefined.js Outdated
message: 'console.log is not a function'
}
);
assert.throws(() => console.log('foo'), TypeError);
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep this one as common.expectsError()

@jackhorton
Copy link
Contributor Author

Updated to expectsError. Just for my own understanding, when is it preferred to use common.expectsError(func, { type }) over assert.throws(func, type)?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

It was originally recommended to use common.expectsError as it was much more powerful than assert.throws. This changed recently and now it is possible to also use a object for the validation. common.expectsError is still better in two small cases: it validates the error type and it allows a RegExp for the messages. There will always be a valid use case for common.expectsError even if those things would be overcome as well: it is possible to use it as callback and that will not be possible with assert.throws.

I personally recommend to always use assert.throws if it is suitable. I might update the docs about it some time soon.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

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

@jackhorton
Copy link
Contributor Author

I have looked at a handful of the CI errors and none of them look related to this change, is that expected? OS X had a missing tool, Linux had a YAML issue, and some others didn't look like they reported why they failed at all.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@jackhorton to some extend: yes. But not as bad as it has happened here. We have to many flakes and other issues with our CI at the moment as it seems.

@lpinca
Copy link
Member

lpinca commented Feb 7, 2018

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
@jackhorton @BridgeAR
test: convert new tests to use error types
1729af2 
PR-URL: nodejs#18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member

Landed in 1729af2 🎉

@BridgeAR BridgeAR closed this Feb 10, 2018
@MylesBorins
Copy link
Member

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@jackhorton
Copy link
Contributor Author

Not sure what the status quo is for this -- theres no functional update, it just makes for fewer special cases in Node-ChakraCore. I think im fine with not backporting it?

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
@jackhorton
test: convert new tests to use error types
ef50323 
PR-URL: nodejs#18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@jackhorton
test: convert new tests to use error types
85728d1 
PR-URL: nodejs#18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@jackhorton @MylesBorins
test: convert new tests to use error types
c90b77e 
Backport-PR-URL: #19447
PR-URL: #18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@jackhorton @BridgeAR
test: convert new tests to use error types
1350802 
PR-URL: nodejs#18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
@jackhorton @MylesBorins
test: convert new tests to use error types
2838f9b 
Backport-PR-URL: #19265
PR-URL: #18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@jackhorton @MayaLekova
test: convert new tests to use error types
f053da0 
PR-URL: nodejs#18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR 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. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jackhorton @BridgeAR @lpinca @MylesBorins @jasnell @cjihrig @nodejs-github-bot