Skip to content

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

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.

@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