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

Closed
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
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

@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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
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 🎉

@MylesBorins
Copy link
Contributor

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
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
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
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
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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
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
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
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
Development

Successfully merging this pull request may close these issues.

7 participants