chore(ci): Set up a separate workflow for flaky tests #1922

flub · 2023-12-22T10:34:46Z

Description

This sets up a separate workflow which runs all tests, including flaky ones. It can be triggered by setting the "flaky-test" label on a PR, which will disable most of the normal CI workflow and instead run just the tests including flaky ones.

The flaky workflow can also be manually triggered on a specific branch.

Notes & open questions

The flaky workflow only runs the tests, none of the other jobs.
Cross-compiled jobs aren't even included. Maybe they should be but right now they don't run tests at all so is pointless.
On any labelling change for the PR the jobs in the workflows will be cancelled if they're still running. I don't think we generally label PRs so that's probably fine.
Flaky tests are going to be carnage for a while!
We have no notification for the cron job results yet. Not sure how to approach this. I'd be fine doing this later.
The workflow_dispatch version (aka manual trigger) is untested, I think we have to wait until it is merged to main to be able to test that?
The UI will now always show all jobs: the skipped flaky jobs on normal PRs and the missing normal jobs on flaky PRs. That's probably fine? A bit noisier though.
The set of "required jobs" has changed again and will need patching up in branch protection.

This implements the bulk of #1919

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.

This sets up a separate workflow which runs all tests, including flaky ones. It can be triggered by setting the "flaky" label on a PR, which will disable the normal CI workflow as well. It can also be manually triggered on a branch.

because github UI does not make it obvious which workflow you're looking at

They are on hosted runners anyway

flub · 2023-12-22T11:00:58Z

@Arqu could you have an initial look at this and point out all silly things I did the hard way? I think this is remarkably close already.

If you have any ideas on notifications for the cron job I'd be happy as well.

flub · 2023-12-22T11:09:12Z

Since the tests run in the flaky workflow are so similar to the tests in normal CI, maybe we should check if this is doable with a reusable-workflow instead? It could take inputs for the few differences (if conditions and test args).

flub · 2023-12-22T13:58:17Z

I rather like this workflow_call version.

But now it opens the door to only having a single ci workflow instead of a separate flaky workflow.

Upsides:

There would only be one entry in the "Actions" list for workflows for a PR. Might be simpler?

Downsides:

The ci action would have a bunch of inputs with defaults. manually triggering could be a bit weird.
The ci action would be cancelled on every label change.

if only americans could spell labelled. Without this when you remove the flaky-test label your PR is stuck in a weird state until you push something again. I think not having that confusion trumps cancelling jobs on labelling changes. Also, I don't think we currently label PRs?

Arqu · 2023-12-22T15:01:01Z

Will take a look tonight.

.github/workflows/flaky.yaml

Arqu · 2023-12-22T21:11:46Z

.github/workflows/tests.yaml

@@ -0,0 +1,182 @@
+# Run all tests, with or without flaky tests.
+
+name: Tests


This is nice.

Arqu · 2023-12-22T21:17:08Z

.github/workflows/tests.yaml

+        esac
+
+    - name: check features
+      if: ${{ ! inputs.flaky }}


is inputs.xyz the correct syntax here. Afaik these should be subclassed to github.event.inputs or github.inputs

i believe inputs is correct: https://docs.github.com/en/actions/learn-github-actions/contexts#inputs-context

Nice, I used to think it was always scoped to the github context.

Arqu · 2023-12-22T21:19:44Z

.github/workflows/ci.yml

-      env:
-        RUST_LOG: "TRACE"
+  tests:
+    if: "! contains(github.event.pull_request.labels.*.name, 'flaky-test')"


I think this will fail outside of a PR context as the github.event.pull_request object will be null.

made it so they always run outside of PR context

Arqu · 2023-12-22T21:20:58Z

.github/workflows/ci.yml

@@ -2,6 +2,7 @@ name: CI

 on:
  pull_request:
+    types: [ 'labeled', 'opened', 'synchronize', 'reopened' ]


Nice idea to use labels to control flow. We could do a no-netsim flag to skip netsim for in progress PRs later.

flub · 2024-01-05T11:17:10Z

@Arqu PTAL, thanks for the suggestions

Arqu · 2024-01-05T11:49:16Z

LGTM now. I guess we need to rebase this and then align branch protections.

flub · 2024-01-05T13:12:42Z

@Arqu merged master (and fixed another bug...)

should be good to merge and adjust the branch protections

flub · 2024-01-05T13:19:40Z

I've also made /~https://github.com/n0-computer/iroh/wiki/Flaky-Tests as a reference for how this is to be used.

## Description This sets up a separate workflow which runs all tests, including flaky ones. It can be triggered by setting the "flaky-test" label on a PR, which will disable most of the normal CI workflow and instead run just the tests including flaky ones. The flaky workflow can also be manually triggered on a specific branch. ## Notes & open questions - The flaky workflow only runs the tests, none of the other jobs. - Cross-compiled jobs aren't even included. Maybe they should be but right now they don't run tests at all so is pointless. - On any labelling change for the PR the jobs in the workflows will be cancelled if they're still running. I don't think we generally label PRs so that's probably fine. - Flaky tests are going to be carnage for a while! - We have no notification for the cron job results yet. Not sure how to approach this. I'd be fine doing this later. - The `workflow_dispatch` version (aka manual trigger) is untested, I think we have to wait until it is merged to main to be able to test that? - The UI will now always show all jobs: the skipped flaky jobs on normal PRs and the missing normal jobs on flaky PRs. That's probably fine? A bit noisier though. - The set of "required jobs" has changed again and will need patching up in branch protection. This implements the bulk of n0-computer#1919 ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Asmir Avdicevic <asmir.avdicevic64@gmail.com>

## Description This sets up a separate workflow which runs all tests, including flaky ones. It can be triggered by setting the "flaky-test" label on a PR, which will disable most of the normal CI workflow and instead run just the tests including flaky ones. The flaky workflow can also be manually triggered on a specific branch. ## Notes & open questions - The flaky workflow only runs the tests, none of the other jobs. - Cross-compiled jobs aren't even included. Maybe they should be but right now they don't run tests at all so is pointless. - On any labelling change for the PR the jobs in the workflows will be cancelled if they're still running. I don't think we generally label PRs so that's probably fine. - Flaky tests are going to be carnage for a while! - We have no notification for the cron job results yet. Not sure how to approach this. I'd be fine doing this later. - The `workflow_dispatch` version (aka manual trigger) is untested, I think we have to wait until it is merged to main to be able to test that? - The UI will now always show all jobs: the skipped flaky jobs on normal PRs and the missing normal jobs on flaky PRs. That's probably fine? A bit noisier though. - The set of "required jobs" has changed again and will need patching up in branch protection. This implements the bulk of #1919 ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Asmir Avdicevic <asmir.avdicevic64@gmail.com>

flub added 3 commits December 22, 2023 11:34

chore(ci): Set up a separate workflow for flaky tests

bf74ff0

This sets up a separate workflow which runs all tests, including flaky ones. It can be triggered by setting the "flaky" label on a PR, which will disable the normal CI workflow as well. It can also be manually triggered on a branch.

Use a label that already exists

e87fece

be consistent

117d77b

flub added the flaky-test label Dec 22, 2023

flub added 3 commits December 22, 2023 11:46

Use correct args

416a643

Rename

da276a1

because github UI does not make it obvious which workflow you're looking at

run clippy, fmt, docs and deny at least

42786b8

They are on hosted runners anyway

flub added 5 commits December 22, 2023 14:47

try using a reusable workflow

a39a0b3

no dashes i guess?

2d9602c

it was indentation of course

64c04e7

I guess required and default don't work well together

48673dd

need to learn a lesson twice do i?

fcdb3db

flub removed the flaky-test label Dec 22, 2023

flub marked this pull request as ready for review December 22, 2023 14:26

flub requested a review from Arqu December 22, 2023 14:26

fixup windows checkout

e372a3c

Arqu reviewed Dec 22, 2023

View reviewed changes

flub added 2 commits January 5, 2024 10:22

Use event_name rather than object contains, trigger for schedule too

c940547

make sure these run outside of PR context

fc60b31

flub added the flaky-test label Jan 5, 2024

Merge branch 'main' into flub/flaky-ci

3b4dd3d

flub added flaky-test and removed flaky-test labels Jan 5, 2024

flub removed the flaky-test label Jan 5, 2024

Unalbeled is it's own event apparently

1eb6915

flub added flaky-test and removed flaky-test labels Jan 5, 2024

Merge branch 'main' into flub/flaky-ci

6f34f73

Arqu approved these changes Jan 8, 2024

View reviewed changes

Arqu merged commit 940b78d into main Jan 8, 2024
27 checks passed

Arqu deleted the flub/flaky-ci branch January 8, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ci): Set up a separate workflow for flaky tests #1922

chore(ci): Set up a separate workflow for flaky tests #1922

flub commented Dec 22, 2023 •

edited

Loading

flub commented Dec 22, 2023

flub commented Dec 22, 2023

flub commented Dec 22, 2023 •

edited

Loading

Arqu commented Dec 22, 2023

Arqu Dec 22, 2023

Arqu Dec 22, 2023

flub Jan 5, 2024

Arqu Jan 5, 2024

Arqu Dec 22, 2023

flub Jan 5, 2024

Arqu Dec 22, 2023

flub commented Jan 5, 2024

Arqu commented Jan 5, 2024

flub commented Jan 5, 2024

flub commented Jan 5, 2024

		@@ -0,0 +1,182 @@
		# Run all tests, with or without flaky tests.

		name: Tests

chore(ci): Set up a separate workflow for flaky tests #1922

chore(ci): Set up a separate workflow for flaky tests #1922

Conversation

flub commented Dec 22, 2023 • edited Loading

Description

Notes & open questions

Change checklist

flub commented Dec 22, 2023

flub commented Dec 22, 2023

flub commented Dec 22, 2023 • edited Loading

Arqu commented Dec 22, 2023

Arqu Dec 22, 2023

Choose a reason for hiding this comment

Arqu Dec 22, 2023

Choose a reason for hiding this comment

flub Jan 5, 2024

Choose a reason for hiding this comment

Arqu Jan 5, 2024

Choose a reason for hiding this comment

Arqu Dec 22, 2023

Choose a reason for hiding this comment

flub Jan 5, 2024

Choose a reason for hiding this comment

Arqu Dec 22, 2023

Choose a reason for hiding this comment

flub commented Jan 5, 2024

Arqu commented Jan 5, 2024

flub commented Jan 5, 2024

flub commented Jan 5, 2024

flub commented Dec 22, 2023 •

edited

Loading

flub commented Dec 22, 2023 •

edited

Loading