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

deps: non-breaking updates #782

Merged
merged 4 commits into from Mar 25, 2020
Merged

deps: non-breaking updates #782

merged 4 commits into from Mar 25, 2020

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Jan 29, 2020

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

  • chalk@3.0.0
  • npm-package-arg@8.0.0
  • semver@7.1.1
  • strip-ansi@6.0.0
  • tar@6.0.0
  • yargs@15.1.0

devDeps

  • prettier@1.19.1

Three modules broke the tests on update:

  • mkdirp@1.0.3
    • metric boatload of usage that breaks. I'm not sure if we'd want to switch to the implementation in Node.js or update it to use the newer version of mkdirp.
  • update-notifier@4.0.0.
  • execa@4.0.0

If it's helpful, I can create issues for these.

Checklist
  • npm test passes

@codecov-io
Copy link

codecov-io commented Jan 29, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@6ae20c8). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #782   +/-   ##
=========================================
  Coverage          ?   96.27%           
=========================================
  Files             ?       27           
  Lines             ?      887           
  Branches          ?        0           
=========================================
  Hits              ?      854           
  Misses            ?       33           
  Partials          ?        0
Impacted Files Coverage Δ
lib/unpack.js 100% <100%> (ø)
lib/temp-directory.js 100% <100%> (ø)
lib/git-clone.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ae20c8...3595613. Read the comment docs.

@bnb
Copy link
Contributor Author

bnb commented Jan 29, 2020

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:

test/npm/test-npm-author-name.js 2> /Users/travis/build/nodejs/citgm/node_modules/strip-ansi/index.js:1
test/npm/test-npm-author-name.js 2> 'use strict';
test/npm/test-npm-author-name.js 2> 
test/npm/test-npm-author-name.js 2> 
test/npm/test-npm-author-name.js 2> TypeError: undefined is not a function
test/npm/test-npm-author-name.js 2>     at /Users/travis/build/nodejs/citgm/node_modules/strip-ansi/index.js:1
test/npm/test-npm-author-name.js 2>     at Script.runInThisContext (vm.js:116:20)
test/npm/test-npm-author-name.js 2>     at Object.Module._compile (/Users/travis/build/nodejs/citgm/node_modules/tap/node_modules/source-map-support/source-map-support.js:525:25)
test/npm/test-npm-author-name.js 2>     at Module._extensions..js (internal/modules/cjs/loader.js:991:10)

@MylesBorins
Copy link
Member

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

  • removing mkdirp (using fs.promises.mkdir /w recursive option)
  • updating xml builder which changes the output of the junit reporter... only changes should be formatting though, other tests were unaffected. Will run CITGM with this branch to confirm

@MylesBorins
Copy link
Member

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/

@MylesBorins
Copy link
Member

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)

Copy link
Member

@BridgeAR BridgeAR left a 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`);
Copy link
Member

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.

Copy link
Member

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

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MylesBorins MylesBorins merged commit fcec11b into nodejs:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants