Switch to GitHub Actions CI by XhmikosR · Pull Request #1600 · cheeriojs/cheerio

XhmikosR · 2020-12-26T12:51:25Z

It seems there are a few issues:

~~.travis.yml was using make travis-test but that was using @npm run test:lint which doesn't exist; I changed it to @npm run lint but the makefile isn't up to date.~~ EDIT: I switched to npm test but IMHO the Makefile should be removed to avoid confusion since it's clearly broken
it seems make travis-test is failing; test-cov also doesn't seem to work. Happy to sort everything out as long as you give me some hints. My suggestion is to move everything to npm scripts only and Actions including coverage.
I added another job with BENCHMARK=true
We can add further Node.js versions, OS etc. Let me know and I can take a stab at it later. I'd just split lint to a separate action or job and run it only once.

Notes:

since Actions don't currently support LTS tags, I explicitly set this to 14 in an env var
I added npm caching which is what Travis CI was doing by default

EDIT: it seems that npm test didn't even run on Travis CI hence why the failures weren't caught sooner.

XhmikosR · 2020-12-27T12:57:12Z

I decided to split the actions so that lint and tests are done faster. So, things that remain:

should we get rid of Makefile completely? I vote for yes since it's outdated and confusing to keep the build scripts in two places
~~should I add more Node.js versions in tests? If so, which ones?~~ EDIT: added 10, 12 and 15; less than 10 fails due to mocha requiring 10
should I add more OS'es in tests? If so, which ones and in which tests?
~~should I add the ability to skip bench if the commit message includes [skip bench] or something like that?~~ EDIT: added this

fb55 · 2020-12-27T14:18:38Z

Tests should be fixed with #1596

XhmikosR · 2020-12-28T06:45:49Z

@fb55 so, how do you think we should proceed with the remaining TODOs? Also, is there something you want me to change?

One can use [bench skip] or [skip bench] in the commit message to skip running the benchmark action

fb55

This looks great. Added some questions.

fb55 · 2020-12-28T06:48:22Z

.github/workflows/ci.yml

+on:
+  push:
+    branches-ignore:
+      - 'dependabot/**'


Will we still run tests for Dependabot PRs with this?

Yup, since depndabot always opens a PR, this just skips the branches.

fb55 · 2020-12-28T06:49:28Z

.github/workflows/ci.yml

+
+      - name: Run Jest with coverage
+        run: npm run test:jest:cov
+        if: matrix.node == 14


Would it make sense to move this to an env var, so that there is only a single reference to the Node version used for coverage?

Can do, how do you want to name the env var? Also, do you want the env var to be in the global scope or under the job?

The pattern in the site action is great, maybe just copy that?

OK, it's just that it might not be self-explanatory using NODE, but I can add a comment.

Also, we could just run jest with --coverage regardless of the Node version, and just run coveralls only on Node.js 14. That would be less lines.

Saw you changed the name to NODE_COV, which should be easy enough to distinguish.

we could just run jest with --coverage regardless of the Node version

I like that idea as well. But the current slight duplication isn't too bad either 🙂

Your call. There isn't so much of a difference, just 3 seconds. I'll leave it as is and you can change it later if needed.

fb55 · 2020-12-28T06:50:44Z

.github/workflows/site.yml

      - uses: actions/setup-node@v2
        with:
-          node-version: 14.x
+          node-version: '${{ env.NODE }}'


This is great 👌

fb55 · 2020-12-28T06:52:21Z

Readme.md

-  <a href="https://travis-ci.org/cheeriojs/cheerio">
-    <img src="https://img.shields.io/travis/cheeriojs/cheerio/main" alt="Travis CI">
+  <a href="https://github.com/cheeriojs/cheerio/actions?query=workflow%3ACI+branch%3Amain">
+    <img src="https://img.shields.io/github/workflow/status/cheeriojs/cheerio/CI/main" alt="Build Status">


Should lint and CI be in the same file (as different jobs), to make this cover both? (That's the current behavior, but I can also see a point about having them be separate.)

The reason I split the actions is so that everything is faster since they run in parallel this way. I doubt having a badge to lint is something that people should care about :)

Wouldn't separate jobs be executed in parallel as well?

Yeah, but I personally find this cleaner. Each workflow does one thing. That being said, the types test could probably be moved to the lint workflow.

Makes sense & I can live with this.

fb55 · 2020-12-28T07:13:39Z

I'm happy to go forward with this. Super excited to see this land.

@XhmikosR The PR is still a draft. Let me know when you feel ready to proceed.

XhmikosR · 2020-12-28T07:14:01Z

I'm still not sure about keeping the Makefile around. It's unnecessary duplication and it seems Makefile has a script that is not present in package.json (xyz).

I'll leave that to you to sort later :)

PS. I'm still being bitten by #1602 since my dev VM is on Windows.

This should allow caches to be shared between workflows.

fb55 · 2020-12-28T07:20:24Z

Good point about the Makefile. Thanks so much for this PR, this moves us forward considerably.

XhmikosR · 2020-12-28T07:24:23Z

@fb55 a few more things to do:

go to the repo apps/integrations/hooks and remove any Travis CI related stuff
adapt any branch protection
go on coveralls and change the threshold for failure to something like 0.5; by default, even a .001 decrease will mark the build as failed

@XhmikosR

As suggested by @XhmikosR in #1600, #1612

@XhmikosR

As suggested by @XhmikosR in #1600, #1612

This was referenced Dec 27, 2020

Use https when possible. #1597

Merged

codeql.yml: fix formatting #1604

Closed

XhmikosR mentioned this pull request Dec 27, 2020

Update Readme.md badges #1611

Merged

XhmikosR added 6 commits December 28, 2020 08:47

Switch to GitHub Actions CI

213e725

Split Actions

8d0c14d

Add a benchmark npm script

b8e4590

CI: add more Node.js versions

cac5d7c

CI: ignore dependabot branches

2715b43

benchmark.yml: allow skipping benchmark run through commit messages

b665e4c

One can use [bench skip] or [skip bench] in the commit message to skip running the benchmark action

fb55 reviewed Dec 28, 2020

View reviewed changes

XhmikosR added 4 commits December 28, 2020 09:06

site.yml: add caching, CI and FORCE_COLOR env vars

8a06ca8

Move Coveralls to Actions

b89890e

Move 14 to an env var

b746dac

Move types test to lint

3536e31

XhmikosR marked this pull request as ready for review December 28, 2020 07:12

Be consistent with the NODE var

0ae7f13

This should allow caches to be shared between workflows.

fb55 merged commit b9453ea into cheeriojs:main Dec 28, 2020

XhmikosR deleted the gh-actions branch December 28, 2020 07:16

fb55 added a commit that referenced this pull request Dec 28, 2020

chore: Remove Makefile, .prettierignore

314430b

As suggested by @XhmikosR in #1600, #1612

fb55 mentioned this pull request Dec 28, 2020

chore: Remove Makefile, .prettierignore #1614

Merged

fb55 added a commit that referenced this pull request Dec 28, 2020

chore: Remove Makefile, .prettierignore (#1614)

36c4c77

As suggested by @XhmikosR in #1600, #1612

5saviahv added the github_actions Pull requests that update Github_actions code label Jan 1, 2021

Uh oh!

Conversation

XhmikosR commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XhmikosR commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fb55 commented Dec 27, 2020

Uh oh!

XhmikosR commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fb55 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fb55 commented Dec 28, 2020

Uh oh!

XhmikosR commented Dec 28, 2020

Uh oh!

fb55 commented Dec 28, 2020

Uh oh!

XhmikosR commented Dec 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

XhmikosR commented Dec 26, 2020 •

edited

Loading

XhmikosR commented Dec 27, 2020 •

edited

Loading

XhmikosR commented Dec 28, 2020 •

edited

Loading