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

chore: test using node 16, remove node 10 #862

Merged
merged 1 commit into from
Jun 12, 2021
Merged

chore: test using node 16, remove node 10 #862

merged 1 commit into from
Jun 12, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 5, 2021

10 is EOL, and 16 is new and shiny? 😀

Possibly blocked by #852?

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #862 (1002ec9) into main (aa7032b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files          31       31           
  Lines         940      940           
=======================================
  Hits          901      901           
  Misses         39       39           

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 aa7032b...1002ec9. Read the comment docs.

@targos
Copy link
Member

targos commented May 5, 2021

Change LGTM but we need to understand why it fails on v16 and fix that

@targos
Copy link
Member

targos commented May 5, 2021

Possibly blocked by #852?

No, that's something else. #852 is about failures of citgm-all runs, not CITGM's own tests.

@targos
Copy link
Member

targos commented May 5, 2021

One of the errors is:
image

/cc @MylesBorins could it be a breaking change from npm v7 ?

Copy link
Member

@mhdawson mhdawson 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
Copy link
Contributor

@targos I'll take a look to see how this broke. We had been testing npm 7 and CITGM when it first landed on master, but it is possible that something regressed.

@MylesBorins
Copy link
Contributor

@targos I'm getting an even earlier failure with v16.1.0 😅

> tap -J --timeout 480 "test/**/test-*.js"

Error: Cannot find module './reports/base'
Require stack:
- /Users/mylesborins/code/citgm/node_modules/jackspeak/noop.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:941:15)
    at resolveFileName (/Users/mylesborins/code/citgm/node_modules/tap/node_modules/resolve-from/index.js:17:39)
    at resolveFrom (/Users/mylesborins/code/citgm/node_modules/tap/node_modules/resolve-from/index.js:31:9)
    at module.exports (/Users/mylesborins/code/citgm/node_modules/tap/node_modules/resolve-from/index.js:34:41)
    at importJSX (/Users/mylesborins/code/citgm/node_modules/tap/node_modules/import-jsx/index.js:24:21)
    at module.exports (/Users/mylesborins/code/citgm/node_modules/tap/node_modules/treport/lib/index.js:13:18)
    at exports.makeReporter (/Users/mylesborins/code/citgm/node_modules/tap/bin/run.js:422:23)
    at runTests (/Users/mylesborins/code/citgm/node_modules/tap/bin/run.js:744:3)
    at mainAsync (/Users/mylesborins/code/citgm/node_modules/tap/bin/run.js:244:5)
    at main (/Users/mylesborins/code/citgm/node_modules/tap/bin/run.js:127:3) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/mylesborins/code/citgm/node_modules/jackspeak/noop.js' ]
}

@BethGriggs
Copy link
Member

@MylesBorins that error looks similar to tapjs/tapjs#624 (comment) / tapjs/tapjs#746

@MylesBorins
Copy link
Contributor

hmmm maybe we need to update tap in CITGM?

@MylesBorins MylesBorins mentioned this pull request May 7, 2021
3 tasks
@MylesBorins
Copy link
Contributor

I have a reproduction for the npm team and we've found where this regressed. Hopefully a fix won't be terribly difficult

@wraithgar
Copy link

This was only working by coincidence pre-7.8.0. Based on how it was "working" before, we may want to think about running commands from a cwd that does not contain a package.

@targos
Copy link
Member

targos commented Jun 12, 2021

Given the results in #866, npm is fixed so I'm landing this PR.

@targos targos merged commit 9d75b2a into nodejs:main Jun 12, 2021
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.

9 participants