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

fix: get Jest green #974

Merged
merged 3 commits into from Sep 21, 2023
Merged

fix: get Jest green #974

merged 3 commits into from Sep 21, 2023

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 12, 2023

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

Ref #959.

I've gone through CITGM smoker builds from 3220 to 3228 (at the time of writing, the last completed run) and checked them all for failures with Jest.

  • 3220
    • due to punycode deprecation, which will be fixed when we upgrade JSDOM to the latest (this is a breaking change, but I'll start landing those this weekend or next week)
    • There's also a no space left on device error. Extra weird here is that it uses 29.6.2, instead of 29.6.4
  • 3221
    • Timed out on fedora-last-latest-x64, fedora-latest-x64, rhel8-x64 and debian10-x64.
    • Same no space left on device for ubuntu1804-64
  • 3222
    • Identical as 3220 - punycode deprecation and no space left on device
  • 3223
    • only running pino
  • 3224
    • only running fastify
  • 3225
    • Timed out on fedora-last-latest-x64, rhel8-x64 and debian10-x64
  • 3226
    • only running pino
  • 3227
    • only running fastify
  • 3228
    • Failed with a mismatch in error message. My guess is that the version check here was fooled by the node version present in the run?

Based on those I think there are 4 kinds of errors

  1. For Node 21, the deprecation of punycode. This is addressed by upgrading JSDOM and patching psl (which has already landed)
  2. no space left on device - I don't think Jest can do anything about that?
  3. 18.x run fools version detection, so we assert on the wrong error message on syntax error in JSON - I'll take a look at this
  4. Timeouts.

Number 4 is where this PR comes in - I'd like to figure out what we can do (I'll dig into the 18.x error later). I think there's 3 options

  1. Skip the platforms that are timing out
  2. Increase the timeout
  3. Only run a subset of the tests

I don't think number 1 is the way to go as, from what I can tell, Jest is only running on those 5 platforms 😅 But maybe if we only run on e.g. Ubuntu (and its disk space issues are sorted) that's good enough for Jest, and it'd still detect issues. Mac and Windows are already skipped, and I don't think wider coverage of Linux platforms provides much additional value.

Is number 2 feasible? The timeout is already at 30 minutes, so I'm not sure...

Number 3 seems the most likely. If that's what we want to do, I can spend some time and try to put together a subset of the tests that exercises the parts of Jest that I think are most likely to discover regressions in Node (usage of vm API (especially the ESM parts of it)).


For now - this PR is opened as a draft and only removes a maintainer that haven't been one for a few years at this point.

@SimenB
Copy link
Member Author

SimenB commented Sep 20, 2023

@Trott this might be something you can merge

@SimenB SimenB marked this pull request as ready for review September 20, 2023 07:24
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (661c192) 96.44% compared to head (e5a3814) 96.44%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #974   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files          28       28           
  Lines        2139     2139           
=======================================
  Hits         2063     2063           
  Misses         76       76           

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

@Trott
Copy link
Member

Trott commented Sep 20, 2023

Testing it on Jenkins here: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3256/

@SimenB
Copy link
Member Author

SimenB commented Sep 20, 2023

timeouts and no space left on device from what I can see

@Trott
Copy link
Member

Trott commented Sep 20, 2023

timeouts and no space left on device from what I can see

AIX refuses to run because of the wrong arch type. I'm guessing (but only guessing) that has to do with compiling core-js. I suppose we could skip AIX for jest. Or maybe there's a way to run tests but skipping the ones that need core-js?

Might the ENOSPC issue sorta kinda be arguably legitimate? When I test locally, the whole jest checkout area is over a gigabyte. While ideally that wouldn't be a problem, these machines are shared and all that..... Ping @nodejs/build

@Trott
Copy link
Member

Trott commented Sep 20, 2023

AIX refuses to run because of the wrong arch type. I'm guessing (but only guessing) that has to do with compiling core-js. I suppose we could skip AIX for jest. Or maybe there's a way to run tests but skipping the ones that need core-js?

It appears that's not due to core-js but because yarn expects to run on little-endian machines only. That's surprising to me, but I can see that the other modules that require "yarn" skip big-endian machines. (Or maybe I'm reversing big-endian and little-endian but you get the idea.)

I'll leave a suggestion.

@Trott
Copy link
Member

Trott commented Sep 20, 2023

Wait, no suggestion needed since it already is set to skip those architectures!

["aix", "s390x", "ppc", "darwin", "win32"],

I guess my configuration for the test run somehow bypassed that. Let me do a full-blown build so we can see it being skipped and see if it's green in the other places.

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3257/

@Trott
Copy link
Member

Trott commented Sep 20, 2023

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3257/

One space failure and one timeout failure, but otherwise, jest passes. Landing (although I might run the failed GitHub Action a few times hoping it passes, but that's not jest's problem).

@Trott
Copy link
Member

Trott commented Sep 21, 2023

GitHub Action passed too. Thanks, @SimenB!

@Trott Trott merged commit 8b6acfb into nodejs:main Sep 21, 2023
11 checks passed
@SimenB SimenB deleted the patch-1 branch September 21, 2023 07:18
@SimenB
Copy link
Member Author

SimenB commented Sep 21, 2023

Thanks again @Trott!

It might make sense to try to run a subset of tests, as timing out after half an hour is quite bad 🤔

@merceyz
Copy link
Member

merceyz commented Sep 21, 2023

because yarn expects to run on little-endian machines only. That's surprising to me
#974 (comment)

@Trott Some context for that:

To ensure reproducibility yarn@>=2 uses libzip and zlib compiled to WASM by Emscripten which doesn't have big-endian support enabled by default.
We've fixed that in yarnpkg/berry#3669 which will be in Yarn v4 when that releases.

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

Successfully merging this pull request may close these issues.

None yet

6 participants