Add Setup Node to Package Tests #399
Conversation
|
Looks like the default NodeJS changed from 16 to 18. Which, major NodeJS bumps imply V8 JS engine C++ code churn, and some compilation errors of our native C/C++ package code against said JS engine's internals. This somewhat frequently happens, compilation errors of our C/C++ code against newer NodeJS major bumps. I think we can pin to older NodeJS (like this PR does). And/or perhaps updating the Researching the cause:
(To view the runner details in a GitHub Actions run, scroll up and expand the first task in the run, "Set up job", then expand the "Runner Image" accordion thing.) In the announcement issue I linked at the top of this comment, they say this, which lines up with the timeline I'm seeing:
So, yeah, we got hit with a new default NodeJS major version in the base image we run on. |
There was a problem hiding this comment.
Approving, since this fixes the problem (I see passing package test runs on this PR to confirm), and reverts to the Node 16 we were using before they changed the default to NodeJS 18 in the CI base images we're using.
(Announcement: actions/runner-images#7002).
Also: We should try updating nan in our lockfiles in a bunch of places and see if we can build on Node 18. Would be nice to have that upgrade path forward opened up/confirmed if possible. (But out of scope for this quick fix PR).
| @@ -144,7 +144,10 @@ jobs: | |||
| steps: | |||
| - name: Checkout the latest code | |||
| uses: actions/checkout@v2 | |||
|
|
|||
| - name: Setup NodeJS | |||
| uses: actions/setup-node@v3 | |||
There was a problem hiding this comment.
You should update any mentions of actions/setup-node@v2-beta to actions/setup-node@v3.
Optional: actions/setup-node@v3 can now cache global packages data. 1 This may speed up your tests. Read the docs to learn more.
Footnotes
There was a problem hiding this comment.
Totally agree that we should also get any usage of setup-node@v2-beta updated. But considering that all PRs have failing tests without this change, I'm thinking it might be best to have that as an additional PR and get these changes merged in as quickly as we can? Since that'd be a pretty easy followup to this one
There was a problem hiding this comment.
Honestly, there's zero backlash in updating to the most recent versions of the actions - but I agree, we probably want to scope them so they are a single commit under their own PR
I'm pretty sure Renovate and Dependabot can actually suggest/PR action updates (I know for a fact Renovate can) - I'd really like to get more widespread adoption of one of them - but that's out of scope for this. Juet bringing it up
There was a problem hiding this comment.
@Spiker985 yeah good point there. I was more worried about the failing PRs and trying to minimize new ones that may appear.
But yes Renovate does actually alert for outdated GitHub Action deps. There's an issue for it somewhere here where the issuer gave an awesome example of how it'd look, and I remember them being there.
There was a problem hiding this comment.
Renovate can update your GitHub Action runners. I don't know if Renovate would update you from v2-beta to v3 though...
Here's the issue about using Renovate on your repository:
|
@DeeDeeG thanks for the review! Considering that all tests are currently failing on our CI without this change and I've got your approval I'll go ahead and merge this one. Thanks! |
|
Do we want to add a node matrix, and we can test 16 and our next targeted version at the same time (18, 19, 21 etc)? If we're having issues with the length/quantity of runs - then I'd say ignore me But if we're, for the most part, not waiting on jobs - it could be helpful to see what completes where |
|
@Spiker985 not really, what matters is the electron-rebuild version only. |
We did in fact have to limit concurrent jobs to 8 for the package tests, since there are ~92 of them and they will eat up our max 20 concurrent jobs at the org level quite quickly. It is a limit we have already run squarely into, IMO. |
|
Occasionally running the build locally on the next Node LTS is a decent idea to get out ahead of these things, and fix whatever issues this raises in a PR so we will be ready ahead of time when this stuff happens. I was thinking of doing it manually locally, but CI for this could work, too. Maybe we can schedule a job every week against NodeJS "LTS" version (latest LTS) that just tries to successfully build the editor with that version of Node? |
This isn't a bad idea honestly. But would probably make more sense to put in the effort once we get past our major bump. Since otherwise we all know for a fact that it will fail 100% of the time until we can do that. |
Seemingly suddenly randomly our package tests are failing across the board.
These unexpected failures can be seen:
markdown-preview,styleguide,wrap-guide#374autocomplete-csscompletions.json#398The fix here was actually seen on #394 as well. Showing that we just needed to add this build step.
Not totally sure what's caused the sudden failure, but as it is an issue that would prevent any PRs from getting merged, it seems important to fix now and ask questions later.
Thanks @mauricioszabo for finding the solution on this one!