-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
fix: default config values for yml files #171
Conversation
@kentcdodds I'm guessing they would pass if you reran the build. EDIT The only way I've been able to consistently reproduce the issue Travis just ran into was by adding the following to the async function runNPS(cwd, args = '') {
// It seems to be somehow linked to one test timing out
// in combination to the other tests taking a while.
if (args === '-c ./package-scripts-with-default.js') {
await new Promise(resolve => setTimeout(resolve, 30000))
} else {
await new Promise(resolve => setTimeout(resolve, 10000))
}
// ... |
Codecov Report
@@ Coverage Diff @@
## master #171 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 382 382
Branches 92 92
=====================================
Hits 382 382
Continue to review full report at Codecov.
|
Updating |
* updated various dev dependencies jest-cli seems to have improved the stability of the tests
@kentcdodds |
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.
Seems good. Thanks! Just one question.
testEnvironment: 'node', | ||
collectCoverageFrom: ['src/**/*.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.
Why did you make these changes?
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'm guessing how jest-cli
resolves paths for coverage changed somehow in the latest version because with this line it wasn't tracking coverage at all.
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.
Huh.... Ok, that's fine. I plan to eventually upgrade this package to use kcd-scripts anyway.
Thanks!
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 know this has already been merged, but as an addition.
Changing the jest config to the following seems to have the
same effect as removing the collectCoverageFrom
line.
module.exports = {
// ...
collectCoverageFrom: ['**/*.js'],
coveragePathIgnorePatterns: [
'/coverage/',
// ...
],
// ...
}
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. I'd rather just upgrade this to use kcd-scripts instead.
What:
All
nps
commands would fail given the following conditions were true:package-scripts.yml
instead of apackage-scripts.js
nps
closes #170
Why:
Current implementation makes it impossible to call
nps
normally without a minor workaround of some sort.e.g.
How:
yml
configurations now go through the same post-process asjs
configurations.Checklist: