Skip to content

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

Merged
targos merged 1 commit intonodejs:mainfrom
SimenB:16
Jun 12, 2021
Merged

chore: test using node 16, remove node 10#862
targos merged 1 commit intonodejs:mainfrom
SimenB:16

Conversation

@SimenB
Copy link
Member

@SimenB SimenB commented May 5, 2021 •
edited
Loading

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 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.85%. Comparing base (aa7032b) to head (1002ec9).
Report is 164 commits behind head on main.

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SimenB SimenB mentioned this pull request May 5, 2021
Closed
@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 ?

mhdawson
mhdawson approved these changes May 6, 2021
View reviewed changes
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

MylesBorins commented May 7, 2021

@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

MylesBorins commented May 7, 2021

@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

BethGriggs commented May 7, 2021

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

@MylesBorins
Copy link
Contributor

MylesBorins commented May 7, 2021

hmmm maybe we need to update tap in CITGM?

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

MylesBorins commented May 7, 2021

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

@wraithgar wraithgar mentioned this pull request May 7, 2021
Merged
@wraithgar
Copy link

wraithgar commented May 7, 2021

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 targos mentioned this pull request Jun 12, 2021
Merged
@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

Reviewers

@targos targos targos approved these changes

@richardlau richardlau richardlau approved these changes

+2 more reviewers

@Trott Trott Trott approved these changes

@mhdawson mhdawson mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@SimenB @codecov-commenter @targos @MylesBorins @BethGriggs @wraithgar @Trott @richardlau @mhdawson