-
Notifications
You must be signed in to change notification settings - Fork 150
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
deps: non-breaking updates #782
Conversation
Codecov Report
@@ Coverage Diff @@
## master #782 +/- ##
=========================================
Coverage ? 96.27%
=========================================
Files ? 27
Lines ? 887
Branches ? 0
=========================================
Hits ? 854
Misses ? 33
Partials ? 0
Continue to review full report at Codecov.
|
As far as I can tell this is not an issue with the module itself, especially since the changelog does not list anything that would result in such a breakage. Happy to revert if folks think it's needed. From Travis:
|
22cbbb9
to
3595613
Compare
Now that we've dropped support for Node.js 8 I've gone ahead and rebased this and update a few other dependencies. Biggest changes include
|
Run of CITGM (no build) to test the output of the XML reporter https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/732/ |
The strip ansi failure on travis is super odd... fails only only on windows and only on travis. (various windows runs on github all passed) |
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.
LGTM with green CI
e.message.includes(nullDevice), | ||
`the message should include the path ${nullDevice}` | ||
); | ||
t.ok(e.message.includes('EEXIST'), `the message should include EEXIST`); |
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.
While being on this, we might also switch to using assert.rejects()
. Otherwise we do not verify that the catch clause is actually reached.
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.
Feel free to push to this PR, although if you do please use the built in tap asserts
https://node-tap.org/docs/api/asserts/#trejectspromise--fn-expectederror-message-extra
TBH the tests across the board could use a refactor so I'd prefer to save fixing this for a larger refactoring
3595613
to
e7bcc45
Compare
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.
LGTM
Dependency updates. Stepped through each outdated dependency individually and ran tests to ensure that there's no breakages.
Module updates that worked without breakages:
deps
All of these are semver major changes
devDeps
Three modules broke the tests on update:
mkdirp@1.0.3
update-notifier@4.0.0
.callback
property was deprecated in 4.0.0. The.fetchInfo()
method is the replacement (yeoman/update-notifier#158).If it's helpful, I can create issues for these.
Checklist
npm test
passes