-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 error message for policy failures #35633
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I'm sure we will both be happy to have these errors reported like this in CI rather than having to log into a failing CI host and poke around the file system.
Oh, sorry, but I forgot that this will also need an entry in test/common/README.md. It would go in the "Common module API" section. The entries are in alphabetical order. Maybe something like this? ### `requireNoPackageJSONAbove()`
Throws an `AssertionError` if a `package.json` file is in any ancestor
directory. Such files may interfere with proper test functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to lib/internal/url.js
look unrelated and accidental?
Co-authored-by: Rich Trott <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
d711850
to
e6be5c7
Compare
Since they were pretty clearly unrelated and accidental, I rebased and added a fixup commit to remove them. |
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #35633 +/- ##
=======================================
Coverage 96.40% 96.40%
=======================================
Files 220 220
Lines 73681 73681
=======================================
+ Hits 71031 71034 +3
+ Misses 2650 2647 -3
Continue to review full report at Codecov.
|
Landed in 5b95f01 |
PR-URL: #35633 Fixes: #35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #35633 Fixes: #35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #35633 Fixes: #35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #35633 Fixes: #35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #35633 Fixes: #35600 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
Policy tests can fail if a `package.json` exists in any of the parent directories above the test. The existing checks are done for the ancestors of the test directory but some tests execute from the tmpdir. PR-URL: #38285 Refs: #38088 Refs: #35600 Refs: #35633 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Policy tests can fail if a `package.json` exists in any of the parent directories above the test. The existing checks are done for the ancestors of the test directory but some tests execute from the tmpdir. PR-URL: #38285 Refs: #38088 Refs: #35600 Refs: #35633 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Policy tests can fail if a `package.json` exists in any of the parent directories above the test. The existing checks are done for the ancestors of the test directory but some tests execute from the tmpdir. PR-URL: #38285 Refs: #38088 Refs: #35600 Refs: #35633 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Policy tests can fail if a `package.json` exists in any of the parent directories above the test. The existing checks are done for the ancestors of the test directory but some tests execute from the tmpdir. PR-URL: #38285 Refs: #38088 Refs: #35600 Refs: #35633 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Policy tests can fail if a `package.json` exists in any of the parent directories above the test. The existing checks are done for the ancestors of the test directory but some tests execute from the tmpdir. PR-URL: #38285 Refs: #38088 Refs: #35600 Refs: #35633 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesfixes: #35600
this doesn't attempt to fix this outside of running node's test suite since we cannot know when a package.json is expected or not except through the policy itself. in theory any test relying on "type" should also use this helper... but that is basically everything so it is fine to omit I think.