build: add daily/on-demand internet test workflow #40086

Trott · 2021-09-11T18:13:03Z

Allows on-demand and nightly runs of the internet tests.

richardlau · 2021-09-11T19:10:26Z

Do we know if the internet test pass on GitHub actions runners?

.github/workflows/test-linux.yml

Trott · 2021-09-11T20:21:48Z

Do we know if the internet test pass on GitHub actions runners?

Once I get the GitHub Action passing without changes to the internet tests, I will add a commit that changes an internet test to check. (Right now, it fails when you don't change the internet tests because grep doesn't find anything and exits with a non-zero return code. Whoops.)

Mesteery · 2021-09-12T07:29:22Z

.github/workflows/test-linux.yml

@@ -31,3 +31,6 @@ jobs:
        run: make build-ci -j2 V=1 CONFIG_FLAGS="--error-on-warn"
      - name: Test
        run: make run-ci -j2 V=1 TEST_CI_ARGS="-p actions"
+      - name: Test Internet
+        # Only run if files in test/internet have changed.
+        run: if [[ $(git diff --name-only origin HEAD -- test/internet) ]]; then make test-internet -j2 V=1; fi


Suggested change

run: if [[ $(git diff --name-only origin HEAD -- test/internet) ]]; then make test-internet -j2 V=1; fi

run: |

if [[ $(git diff --name-only origin HEAD -- test/internet) ]]; then

make test-internet -j2 V=1;

fi

The only problem is that it will be green if there are no changes in test/internet, so it might not be practical to know if it really ran or not IMO. A solution would be (with fetch-depth: 0):

- name: Get changed test/internet files id: check-internet run: echo "::set-output name=changed::$(git diff --name-only origin/${{ github.base_ref || 'master' }} HEAD -- test/internet)" - name: Test Internet # Only run if files in test/internet have changed. if: steps.check-internet.outputs.changed run: make test-internet -j2 V=1

But at this point, an action like https://github.com/tj-actions/changed-files could even be used.

Refs (-j2 -> -j): #40080 (comment)

The only problem is that it will be green if there are no changes in test/internet, so it might not be practical to know if it really ran or not IMO.

I'm OK with that, especially if it keeps the workflow simple and light on dependencies.

Refs (-j2 -> -j): #40080 (comment)

I left a comment there but I'm pretty sure we don't want to do that.

The multi-line suggestion is a good one. Right now, this isn't working though because of the way the workflow fetches from origin. So that if stuff might be going away. If not, I'll do it your way for sure. Thanks! Pondering the right solution...

What made you add --exit-code?

What made you add --exit-code?

git diff always exits with code 0, even if diff is empty.
With --exit-code it exits with code 1 if changes, otherwise 0.

I don't think we need that. The if [[ $( COMMAND ) ]] means "true if there is any output from COMMAND, otherwise false".

Mesteery · 2021-09-12T07:49:29Z

For fatal: bad revision 'origin', maybe fetch-depth: 0 in checkout would work?
By the way git diff --name-only HEAD^ HEAD -- test/internet seems to work better.

Trott · 2021-09-12T11:48:26Z

For fatal: bad revision 'origin', maybe fetch-depth: 0 in checkout would work?

Yes, but I'm not sure how much overhead (and therefore additional time waiting for the job to complete) it would add. Do you have any idea?

By the way git diff --name-only HEAD^ HEAD -- test/internet seems to work better.

That will only check the last commit. I want to check all commits in a PR.

Mesteery · 2021-09-12T12:23:00Z

Yes, but I'm not sure how much overhead (and therefore additional time waiting for the job to complete) it would add. Do you have any idea?

Approximately 1 minute vs 30 seconds for checkout.
I think that in total, it will only take a few minutes longer than at present.

Trott · 2021-09-12T12:39:25Z

By the way git diff --name-only HEAD^ HEAD -- test/internet seems to work better.

That will only check the last commit. I want to check all commits in a PR.

Argh, although I'm now realizing that comparing against origin/HEAD could have spurious results if the pull request branch isn't up to date with origin/HEAD. I guess I could rebase inside the workflow but that seems like it might have its own problems (like the entire workflow failing if the rebase fails--is that a feature or a bug?).

Mesteery · 2021-09-12T12:47:44Z

We can simply compare between the SHA of the previous ("before"/"base") commit and the HEAD. This works for pull requests and pushes (and it's provided in github context).

Mesteery · 2021-09-12T13:14:49Z

      - name: Get changed test/internet files
        id: check-internet
        run: |
          changes=$(git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD -- test/internet)
          [ $? -ne 0 ] && exit $?
          echo "::set-output name=changes::$changes"
      - name: Test Internet
        # Only run if files in test/internet have changed.
        if: steps.check-internet.outputs.changes
        run: make test-internet -j2 V=1

This should normally work.
In fact, finally, this also does not work well for push.

Trott · 2021-09-12T13:35:20Z

This started off as such a simple addition, but it's starting to seem like it will need a fair amount of stuff to make it work robustly and I wonder if this isn't the way to do it but to instead put it in its own workflow file.

Mesteery · 2021-09-12T13:39:00Z

I also think this is the best solution.

Trott · 2021-09-12T15:23:05Z

Added it as a daily/on-demand workflow for now. We can later improve it to run on pull requests and pushes when appropriate only (or not).

Trott · 2021-09-12T15:28:34Z

Here's the workflow running on my fork: https://github.com/Trott/io.js/actions/runs/1226807643

Mesteery · 2021-09-12T15:38:15Z

.github/workflows/test-internet.yml

+on:
+  workflow_dispatch:
+  schedule:
+    - cron: "5 0 * * *"


Added it as a daily/on-demand workflow for now. We can later improve it to run on pull requests and pushes when appropriate only (or not).

I want to say that it's almost useless if you do it that way. It would just be impossible to run this workflow on a PR that changes the internet tests, and that's a shame, it loses all its usefulness IMO.

Suggested change

on:

workflow_dispatch:

schedule:

- cron: "5 0 * * *"

on:

workflow_dispatch:

schedule:

- cron: '5 0 * * *'

pull_request:

types: [opened, synchronize, reopened, ready_for_review]

paths:

- test/internet/**

- deps/cares/**

- lib/dns.js

- lib/internal/dns/**

- src/cares*

- lib/dgram.js

- lib/internal/dgram.js

- src/*udp*

- lib/*http*

- lib/internal/http.js

- lib/internal/http2/**

- src/*http*

- src/tcp*

- deps/ng*/**

push:

paths:

- test/internet/**

- deps/cares/**

- lib/dns.js

- lib/internal/dns/**

- src/cares*

- lib/dgram.js

- lib/internal/dgram.js

- src/*udp*

- lib/*http*

- lib/internal/http.js

- lib/internal/http2/**

- src/*http*

- src/tcp*

- deps/ng*/**

branches:

- master

- main

- canary

- v[0-9]+.x-staging

- v[0-9]+.x

The additions you're proposing here may be worth doing, but we should probably keep it on a schedule too. Other changes may break internet tests (such as changes to cares or dns.js) and we might not notice for a long time. If it's a month later, it will be very useful to have daily results to go back to so we can immediately narrow the issue down to a single day's worth of commits.

My idea was that someone can manually launch the job on PRs if the PR branch is in the repo. (Or they can launch it on their own branch if it's in their repo.)

By the way, the "run once a day and run it manually" mirrors what we do on Jenkins now, so they idea is that this would allow us to get rid of the Jenkins job.

(Although it does seem like the Jenkins job runs much faster. Maybe I'm doing something wrong in the workflow and there's a way to have a persistent compile cache to speed things up or something.)

I have updated the suggestion.

If it's OK with you, I think my preference is to land this as-is and then incrementally add these files/paths over time. We've gone years and years without having these jobs run automatically, and there's always been certain concerns around running them routinely in CI. These concerns (don't want to hit a DNS, web, or other external service too much from our CI, for example) may not be well-founded, but just in case, I think an incremental approach is still the way to go.

It's OK with me.

github-actions · 2021-09-13T18:41:56Z

Landed in e016cc7...ed556c0

PR-URL: #40086 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Sep 11, 2021

lpinca approved these changes Sep 11, 2021

View reviewed changes

mscdex reviewed Sep 11, 2021

View reviewed changes

.github/workflows/test-linux.yml Outdated Show resolved Hide resolved

Trott force-pushed the run-internet branch from 0d2af17 to 014db57 Compare September 11, 2021 23:20

Mesteery reviewed Sep 12, 2021

View reviewed changes

Trott closed this Sep 12, 2021

build: add daily/on-demand internet test workflow

19311e0

Trott reopened this Sep 12, 2021

Trott force-pushed the run-internet branch from 014db57 to 19311e0 Compare September 12, 2021 15:23

fixup! build: add daily/on-demand internet test workflow

22cb29f

Mesteery reviewed Sep 12, 2021

View reviewed changes

Trott changed the title ~~build: run internet tests in GitHub Actions if changed~~ build: add daily/on-demand internet test workflow Sep 12, 2021

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2021

targos approved these changes Sep 13, 2021

View reviewed changes

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2021

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2021

github-actions bot closed this Sep 13, 2021

nodejs-github-bot pushed a commit that referenced this pull request Sep 13, 2021

build: add daily/on-demand internet test workflow

ed556c0

PR-URL: #40086 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs pushed a commit that referenced this pull request Sep 21, 2021

build: add daily/on-demand internet test workflow

23b0871

PR-URL: #40086 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs pushed a commit that referenced this pull request Sep 21, 2021

build: add daily/on-demand internet test workflow

c804d07

PR-URL: #40086 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs mentioned this pull request Sep 21, 2021

v16.10.0 proposal #40175

Merged

1 task

Trott deleted the run-internet branch September 25, 2022 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add daily/on-demand internet test workflow #40086

build: add daily/on-demand internet test workflow #40086

Trott commented Sep 11, 2021 •

edited

richardlau commented Sep 11, 2021

Trott commented Sep 11, 2021

Mesteery Sep 12, 2021 •

edited

Trott Sep 12, 2021

Trott Sep 12, 2021

Trott Sep 12, 2021

Mesteery Sep 12, 2021

Trott Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021 •

edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021

Trott commented Sep 12, 2021 •

edited

Trott commented Sep 12, 2021

Mesteery Sep 12, 2021 •

edited

Trott Sep 12, 2021 •

edited

Trott Sep 12, 2021

Trott Sep 12, 2021

Mesteery Sep 12, 2021

Trott Sep 12, 2021

Mesteery Sep 13, 2021

github-actions bot commented Sep 13, 2021

-on:
-  workflow_dispatch:
-  schedule:
-    - cron: "5 0 * * *"
+on:
+  workflow_dispatch:
+  schedule:
+    - cron: '5 0 * * *'
+  pull_request:
+    types: [opened, synchronize, reopened, ready_for_review]
+    paths:
+      - test/internet/**
+      - deps/cares/**
+      - lib/dns.js
+      - lib/internal/dns/**
+      - src/cares*
+      - lib/dgram.js
+      - lib/internal/dgram.js
+      - src/*udp*
+      - lib/*http*
+      - lib/internal/http.js
+      - lib/internal/http2/**
+      - src/*http*
+      - src/tcp*
+      - deps/ng*/**
+  push:
+    paths:
+      - test/internet/**
+      - deps/cares/**
+      - lib/dns.js
+      - lib/internal/dns/**
+      - src/cares*
+      - lib/dgram.js
+      - lib/internal/dgram.js
+      - src/*udp*
+      - lib/*http*
+      - lib/internal/http.js
+      - lib/internal/http2/**
+      - src/*http*
+      - src/tcp*
+      - deps/ng*/**
+    branches:
+      - master
+      - main
+      - canary
+      - v[0-9]+.x-staging
+      - v[0-9]+.x

build: add daily/on-demand internet test workflow #40086

build: add daily/on-demand internet test workflow #40086

Conversation

Trott commented Sep 11, 2021 • edited

richardlau commented Sep 11, 2021

Trott commented Sep 11, 2021

Mesteery Sep 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Sep 12, 2021 • edited

Choose a reason for hiding this comment

Mesteery commented Sep 12, 2021 • edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021 • edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021 • edited

Mesteery commented Sep 12, 2021 • edited

Trott commented Sep 12, 2021

Mesteery commented Sep 12, 2021

Trott commented Sep 12, 2021 • edited

Trott commented Sep 12, 2021

Mesteery Sep 12, 2021 • edited

Choose a reason for hiding this comment

Trott Sep 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2021

Trott commented Sep 11, 2021 •

edited

Mesteery Sep 12, 2021 •

edited

Trott Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Mesteery commented Sep 12, 2021 •

edited

Trott commented Sep 12, 2021 •

edited

Mesteery Sep 12, 2021 •

edited

Trott Sep 12, 2021 •

edited