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

Node.js 16 failures #852

Closed
targos opened this issue Mar 7, 2021 · 24 comments
Closed

Node.js 16 failures #852

targos opened this issue Mar 7, 2021 · 24 comments

Comments

@targos
Copy link
Member

targos commented Mar 7, 2021

I started a run (with citgm's main branch) against Node.js master to check the current status: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2643/

@targos targos mentioned this issue Mar 15, 2021
6 tasks
@devinivy
Copy link

We published hapi 20.1.2, and it should contain fixes for those issues 👍

@richardlau
Copy link
Member

New run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2650/

@richardlau node-report seems a bit too strict in its tests for version numbers: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2650/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/node_report_v2_2_11/

node-report is comparing the versions of Node.js dependencies in the report to process.versions -- the failure is occurring since we use each value in process.versions to construct a regular expression and the version string for OpenSSL for the fork we're now using contains a + character which needs to be escaped for use in a regular expression. Raised nodejs/node-report#147 to fix.

@MylesBorins
Copy link
Contributor

@targos the npm failure seems to be related to the pre-release version of Node.js causing weird issues with the npm install phase.

@BethGriggs
Copy link
Member

BethGriggs commented Apr 7, 2021

CITGM isn't particularly healthy on the v16.0.0 proposal (132 failures). v16.0.0 has not diverged from master yet (just slightly behind), so I suspect runs on master yield similar results.

Run on rc.2 (3bc20dc) - https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2671/testReport/

Probably best to start by capturing the failures that are occurring on many platforms:

Old failures

@richardlau
Copy link
Member

* [ ]  undici-v3.3.3
  
  * [Link](https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2671/nodes=debian9-64/testReport/(root)/citgm/undici_v3_3_3/)

At least one of the failures,

     # Subtest: 20 times GET with pipelining 10
         1..61
         not ok 1 - (unnamed test)
           ---
           code: DEP0111
           ...

Is the process.binding deprecation which was removed from undici in nodejs/undici#564 but hasn't been released.
Refs: nodejs/node#37813 (comment).

@BethGriggs
Copy link
Member

I can reproduce the npm-v7.8.0 failures locally on the v16.0.0-proposal build 🤔

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

My git bisect is pointing to the V8 9.0 upgrade nodejs/node#37587 - as the last known good commit to build/pass npm test appears to be the previous commit, 802b3e7cf9. But I haven't yet convinced myself that #37587 is responsible (or why).

/cc @nodejs/npm

@ruyadorno
Copy link
Member

hi @BethGriggs! That is looking more like a https://github.com/tapjs/node-tap (which is the test framework we're using) problem than a npm one, we'll dig into it!

@BethGriggs
Copy link
Member

Thanks @ruyadorno. From further digging around tap I found tapjs/tapjs#624 - which shows a similar/relevant error. As suggested there, passing --reporter classic does run through the test suite and the npm suite passes. So I suspect this issue would impact more than just npm's test suite.

@BethGriggs
Copy link
Member

BethGriggs commented Apr 20, 2021

Results from the v16.0.0-rc.5 - i've omitted ones that are only on one platform and familiar timeout flakes.

Edit: CITGM run link https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2679/testReport/

@targos
Copy link
Member Author

targos commented Apr 20, 2021

citgm.jest-v26.6.3
Received message: [31m"exports.createRequireFromPath is not a function"

@SimenB

@SimenB
Copy link
Member

SimenB commented Apr 20, 2021

Thanks for the heads up! From nodejs/node#37201 I assume - will tweak our tests (we have some feature detection so it's a fallback if createRequire isn't there, seems we've messed that up).

EDIT: Actually, this is the feature detection working correctly, but the test didn't handle it missing in our implementation when it was missing in Node's implementation 😅

Fix: jestjs/jest@a5ac3c5

Since the error is just in tests and we're currently in a pre-release, would it be possible to have CITGM install from master instead of published release? Finagling branches and releases in the monorepo is more painful than it needs to be, and since the change is only in a test I'd like to avoid it 🙈


v16 added to CI, fwiw

@BethGriggs
Copy link
Member

citgm.fastify-v3.14.2
Various - including 'not ok 7 - test/close-pipelining.test.js'

@mcollina just FYI in case this isn't on your radar yet, fastify is failing on master/v16.0.0-rc.5 - link to CITGM error

@ljharb
Copy link
Member

ljharb commented Apr 20, 2021

Can you link to the failure for resolve, so i can look into it?

@targos
Copy link
Member Author

targos commented Apr 20, 2021

@mcollina
Copy link
Member

nodejs/undici#754 fixes the problem for Fastify.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2021

@targos thanks! has anything changed with symlinks in node 16 that i could investigate? I don't have a windows machine to test on.

@BethGriggs
Copy link
Member

@ljharb I skimmed back a few CITGM builds and noticed the resolve failure was also present on runs for v15.13.0 and v14.16.1. I'm not sure how far back the failure goes, but I'm thinking that it's possibly related to our CITGM set up.

Would any one with a Windows box mind confirming if resolve's tests pass locally (both directly and through CITGM)?

@targos
Copy link
Member Author

targos commented Apr 22, 2021

It's possible that the Windows machines in CI are not configured in development mode. This is mandatory to be able to create symlinks from a non-administrator account.

@ljharb
Copy link
Member

ljharb commented Apr 22, 2021

Thanks, hopefully that’s the case :-)

(Some tooling that’d be nice is auto-filing an issue when something fails a few times in a row, and tagging the maintainers of that package)

@ljharb
Copy link
Member

ljharb commented Jun 14, 2021

@targos Are the resolve failures on windows … resolved?

@targos
Copy link
Member Author

targos commented Jun 14, 2021

@ljharb no, resolve still fails on Windows

@ljharb
Copy link
Member

ljharb commented Jun 14, 2021

Is there an open issue to track that, since it seems to be CITGM-specific?

@targos
Copy link
Member Author

targos commented Jun 14, 2021

I don't think so. It should be a nodejs/build issue about enabling symlinks on Windows machines

gfyoung added a commit to forking-repos/node-ignore that referenced this issue Nov 3, 2021
Also adds 16.x (latest LTS) to testing suite.

xref: tapjs/tapjs#624

The issue is closed, but it is still being reported.

xref: nodejs/citgm#852 (comment)

This is the fix being used in this commit.
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

No branches or pull requests

9 participants