-
Notifications
You must be signed in to change notification settings - Fork 404
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
chore: Updated config integration tests to node:test #2777
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
- Coverage 97.26% 97.21% -0.05%
==========================================
Files 294 294
Lines 46315 46315
==========================================
- Hits 45047 45025 -22
- Misses 1268 1290 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -3,3 +3,4 @@ files: | |||
# We can't do a simple '**/*.test.js' because the "uninstrumented" suite | |||
# includes a `node_modules` directory that includes several `.tests.js` files. | |||
- 'test/integration/*.test.js' | |||
- 'test/integration/config/*.test.js' |
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.
shouldn't we just change the line above to be test/integration/**/*.test.js
?
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.
No. The comment directly preceding these entries explains why.
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.
but I don't see any files in uninstrumented with .test.js
in it
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.
oh, they are in node_modules. I'd prefer we add a glob to exclude those instead of specifying every folder we have in integration tests, this will become unwieldy
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.
@bizob2828 okay, we'll need mcollina/borp#127 to be released in order to do that.
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.
i won't hold this up. we can also upgrade and remove these entries
9b280b6
to
cc17c24
Compare
This PR resolves #2767.