Add Setup Node to Package Tests

[confused-Techie]

Seemingly suddenly randomly our package tests are failing across the board.

These unexpected failures can be seen:

• Tests for ruby #394
• Bundle markdown-preview, styleguide, wrap-guide #374
• Add Automated updated of autocomplete-css completions.json #398

The 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!

[DeeDeeG]

Looks like the default NodeJS changed from 16 to 18.

actions/runner-images#7002

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 nan package in a bunch of places in our lockfiles will be sufficient to get over this hurdle, without pinning to Node 16, can build for Node 18 too?

Researching the cause:

• The last good-looking Package Tests run had this runner image: Image: ubuntu-20.04; Version: 20230206.1
  • Included Software: https://github.com/actions/runner-images/blob/ubuntu20/20230206.1/images/linux/Ubuntu2004-Readme.md
    • Default NodeJS: Node.js 16.19.0
  • The run https://github.com/pulsar-edit/pulsar/actions/runs/4198117872/jobs/7281525186
    • Feb 16, 2023, 3:57 PM EST
• The next Package Tests run was all failing, and had this runner image: Image: ubuntu-20.04; Version: 20230217.1
  • Included Software: https://github.com/actions/runner-images/blob/ubuntu20/20230217.1/images/linux/Ubuntu2004-Readme.md
    • Default NodeJS: Node.js 18.14.1
  • The run: https://github.com/pulsar-edit/pulsar/actions/runs/4239398276/jobs/7367503461
    • Feb 21, 2023, 11:25 PM EST

(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:

Breaking changes

We are going to set Nodejs 18 as the default version across all VM images.

Target date

Image deployment is starting on February 13 and will take 3-4 days.

So, yeah, we got hit with a new default NodeJS major version in the base image we run on.

[DeeDeeG]

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).

[HonkingGoose]

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. ¹ This may speed up your tests. Read the docs to learn more.

Footnotes

1. https://github.com/actions/setup-node#caching-global-packages-data ↩

[confused-Techie]

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

[Spiker985]

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

[confused-Techie]

@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.

[HonkingGoose]

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:

• Get Renovate bot running on this repository with a basic config #321

[confused-Techie]

@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!

[Spiker985]

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

[mauricioszabo]

@Spiker985 not really, what matters is the electron-rebuild version only.

[DeeDeeG]

But if we're, for the most part, not waiting on jobs - it could be helpful to see what completes where

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.

[DeeDeeG]

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?

[confused-Techie]

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.