build: run e2e tests against node 22 #39373
Conversation
gatsbot
bot
added
the
status: triage needed
For some reason the yarn install was being nondeterministic and we'd sometimes end up with 1.0.7 being resolved here and other times 1.0.1. The former had a new required `attributes` property and the latter had no such type, so we'd end up with a type error when it resolved to the former (and if we made this change only, we'd have type errors if it resolved to the latter). So just bump to 1.0.7+ and add the property.
serhalp
force-pushed
the
ci/e2e-node-22
branch
from
80cd775 to
5dc38ab
Compare
[email protected] bumped to [email protected] which bumped to joi@8 which dropped support for node 18 See bahmutov/start-server-and-test#407 and follow the crumbs. 😭 These test fixtures don't use lockfiles.
serhalp
force-pushed
the
ci/e2e-node-22
branch
from
0ef151a to
ff9af55
Compare
serhalp
force-pushed
the
ci/e2e-node-22
branch
from
ff9af55 to
6e5273b
Compare
serhalp
force-pushed
the
ci/e2e-node-22
branch
from
6e5273b to
e5af37a
Compare
serhalp
changed the title
serhalp
removed
the
status: triage needed
| VERBOSE: 1 | ||
| e2e18: | ||
| docker: | ||
| - image: "cypress/browsers:node-18.20.3-chrome-125.0.6422.141-1-ff-126.0.1-edge-125.0.2535.85-1" |
There was a problem hiding this comment.
For the record, two existing e2e jobs were hardcoding this exact image, which includes node.js 18.20.3, whereas the rest of the e2e jobs were using cypress/browsers:node18.6.0-chrome105-ff104, i.e. node.js 18.6.0... and the prior bootstrap step for each of these always uses 18.2.0. It's a bit of a mess due to various minimum versions needed for various things. It didn't seem worth coming up with a more convoluted setup to override the executor for these two tests, so I made this image the one used for node 18 for all e2e test suites.
| - run: | ||
| name: Step debug info | ||
| command: | | ||
| yarn list react | ||
| yarn why lmdb-store |
There was a problem hiding this comment.
unrelated, but this old debug junk was several years old
| matrix: | ||
| parameters: | ||
| node_version: ["18.2.0", "22.13"] | ||
| e2e_executor_suffix: ["18", "22"] | ||
| exclude: | ||
| - node_version: "18.2.0" | ||
| e2e_executor_suffix: "22" | ||
| - node_version: "22.13" | ||
| e2e_executor_suffix: "18" | ||
| requires: | ||
| - bootstrap-18.2.0 | ||
| - bootstrap-<< matrix.node_version >> |
There was a problem hiding this comment.
Unfortunately every approach we could come up with here was lousy. It's surprisingly difficult to accomplish what we want here within the CircleCI config constraints (have a matrix of two coupled vars, or a matrix and then some param or something derived by string manipulation or manual mapping from param values...). 🤷🏼 This works.
serhalp
changed the title
| test_path: e2e-tests/visual-regression | ||
| - store_artifacts: | ||
| path: e2e-tests/visual-regression/__diff_output__ | ||
| path: e2e-tests/visual-regression/<< parameters.e2e_executor_suffix >>/__diff_output__ |
There was a problem hiding this comment.
I think path changes for store_artifacts and store_test_results are not exactly right ... but some of them probably were not right to begin with
In particular this change result in changing artifact upload from:
Uploading /root/project/e2e-tests/visual-regression/__diff_output__ to e2e-tests/visual-regression/__diff_output__
No artifact files found at /root/project/e2e-tests/visual-regression/__diff_output__
Total size uploaded: 0 B
to
Uploading /root/project/e2e-tests/visual-regression/22/__diff_output__ to e2e-tests/visual-regression/22/__diff_output__
No artifact files found at /root/project/e2e-tests/visual-regression/22/__diff_output__
Total size uploaded: 0 B
so it changes path to look for artifacts to store and not path under which they are stored. For that I think we would need to use destination property https://circleci.com/docs/reference/configuration-reference/#storeartifacts, but I think none of this is needed because artifacts are scoped to the job and we have separate job for 18 and 22.
This particular case is somewhat annoying to check if it works correctly because __diff_output__ directory will only have files in it when image snapshots are not matching, but unless we changed directory where things will be stored within the job - the change is not right.
The store_test_results ones are somewhat similar, but they do not have a destination option - but that's probably just fine, because test results are scoped to a job anyway so you don't even need to try to differentiate between node 18 and 22 job variants here as they are already separate.
Just in store_test_results case I think it was not right to begin with because before the change this step was behaving like so:
Unable to save test results from /root/project/e2e-tests/visual-regression/cypress/results
Error error accessing path "/root/project/e2e-tests/visual-regression/cypress/results": stat /root/project/e2e-tests/visual-regression/cypress/results: no such file or directory
Found no test results, skipping
And with the change ... it still can't find result, just in different dir
Unable to save test results from /root/project/e2e-tests/visual-regression/cypress/22/results
Error error accessing path "/root/project/e2e-tests/visual-regression/cypress/22/results": stat /root/project/e2e-tests/visual-regression/cypress/22/results: no such file or directory
Found no test results, skipping
But as I mentioned - they were probably not right before anyway, so I would personally just not change those store_test_results and store_artifacts settings?
Afaik no automation relies on this and the artifacts are only meant for potential debugging of test failures and test results are meant to make use of a bit nicer UI to present test results - for example like so for unit tests:
or
or (exposing some details in preview, so it's not just "failed", but also what failed)
or produce data for https://circleci.com/docs/guides/insights/insights-tests/ (which I don't think we really use )
There was a problem hiding this comment.
Fixing in a follow-up PR!
See #39373 (comment). - use correct unscoped paths for both `store_artifacts` and `store_test_results` - create missing directories to avoid error noise
* ci: fix e2e debug output setup See #39373 (comment). - use correct unscoped paths for both `store_artifacts` and `store_test_results` - create missing directories to avoid error noise * ci: remove unused Cypress Cloud setup It adds unnecessary noise and complexity to our CI config.
Description
In #39349 we added support for node.js 22. In that PR we started running unit tests against node.js 18 + 20 + 22 and integration tests against 18 and 22, but we left the e2e tests as is, running only against node.js 18.
This PR updates e2e tests to run against both 18 and 22. There were a few complexities here — I'll comment on those inline.
Unfortunately, a few things had broken on the main branch recently, so this PR also fixes those:
start-server-and-testin e2e test fixtures to avoid picking up a breaking change in a transitive dependency joi@18, which dropped node.js 18 support but was then bumped in another dependency without a major.To do