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

lookup: add resolve #775

Merged
merged 1 commit into from
Jan 7, 2020
Merged

lookup: add resolve #775

merged 1 commit into from
Jan 7, 2020

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Dec 17, 2019

Running resolve's tests will help avoid problems like nodejs/node#30963 from occurring silently.

Checklist
  • npm test passes
  • tests are included
  • contribution guidelines followed
    here

Hard Requirements

  • Module source code must be on Github.
  • Published versions must include a tag on Github
  • The test process must be executable with only the commands npm install && npm test or (yarn install && yarn test) using the tarball downloaded from the Github tag mentioned above
  • The tests pass on supported major release lines
  • The maintainers of the module remain responsive when there are problems
  • At least one module maintainer must be added to the lookup maintainers field

Soft Requirements

At least one of:

  • The module must be actively used by the community OR
  • The module must be heavily depended on OR
  • The module must cover unique portions of our API OR
  • The module fits into a key category (e.g. Testing, Streams, Monitoring, etc.) OR
  • The module is under the Node.js foundation Github org OR
  • The module is identified as an important module by a Node.js Working Group

Tests currently fail on v13.4 due to nodejs/node#30963 not being included in it; the next release of resolve will fix that.

Running `resolve`'s tests will help avoid problems like nodejs/node#30963 from occurring silently.
@ljharb ljharb changed the title Resolve lookup: add resolve Dec 17, 2019
package.json Outdated Show resolved Hide resolved
lib/lookup.json Outdated Show resolved Hide resolved
Copy link
Contributor

@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 assuming tests pass. Also would be good to test on the various release lines and ensure we appropriately update the flaky / skip tags

@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #775 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #775   +/-   ##
=======================================
  Coverage   95.49%   95.49%           
=======================================
  Files          27       27           
  Lines         888      888           
=======================================
  Hits          848      848           
  Misses         40       40

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 37d0b2f...76a3a45. Read the comment docs.

@richardlau
Copy link
Member

CITGM-smoker-nobuild runs:
Node.js 8: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/691/
Node.js 10: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/692/
Node.js 12: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/693/
Node.js 13: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/694/
Node.js 14 nightly (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/696/

Some Windows failures on Node.js 8/10.
All platforms passed on Node.js 12 and 13.
Node.js 13.4.0 passed (was expected to fail per the initial post?). Tests fail against the most recent nightly from master.

@ljharb
Copy link
Member Author

ljharb commented Dec 18, 2019

I’ve released v1.14 of resolve since the OP, so node v13.14 should pass now :-)

@ljharb
Copy link
Member Author

ljharb commented Dec 28, 2019

Everything's green; is this good to land?

@richardlau
Copy link
Member

Everything's green; is this good to land?

As noted above there were some failures on Windows with Node.js 8 and 10. We can probably ignore 8 since that’s imminently End-of-Life.

@ljharb
Copy link
Member Author

ljharb commented Dec 29, 2019

Where are those failures indicated? What i saw was intermittent timeouts on npm install, resolved on rerun, nothing related to this PR.

@richardlau
Copy link
Member

Reran CITGM-smoker-nobuild for Node.js 10 (old CI links are too old so can't drill down into the failures):
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/705/

All green, I'll merge.

@richardlau richardlau merged commit 4e4a773 into nodejs:master Jan 7, 2020
@ljharb ljharb deleted the resolve branch January 7, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants