tests: split workflow into smaller parts #14689

maykathm · 2024-11-05T15:00:53Z

This separates out the following jobs into separate workflows:

snap builds
unit tests (including cross-distro)
static checks
spread

I thought it could be beneficial, from a maintenance perspective, to keep run labels in the top-level test.yaml file. That way the top-level yaml takes on the responsibility of deciding OSes and orchestrating and parameterizing jobs while the called workflows is only responsible for its jobs logic, given the inputs.

I kept matrix logic at the test.yaml level to allow for greater future reuse.

For example, instead of using a matrix strategy, one can simply call the workflow with the various parameters. So while the following works:

  unit-tests-cross-distro:
    uses: ./.github/workflows/unit-tests.yaml
    needs: [static-checks]
    with:
      runs-on: ubuntu-latest
      distro: ${{ matrix.distro }}
    strategy:
      fail-fast: false
      matrix:
        distro:
          - fedora:latest
          - opensuse/tumbleweed

one can also write:

  unit-tests-fedora:
    uses: ./.github/workflows/unit-tests.yaml
    needs: [static-checks]
    with:
      runs-on: ubuntu-latest
      distro: fedora:latest

  unit-tests-opensuse:
    uses: ./.github/workflows/unit-tests.yaml
    needs: [static-checks]
    with:
      runs-on: ubuntu-latest
      distro: opensuse/tumbleweed

codecov · 2024-11-05T15:26:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.98%. Comparing base (96ea7b0) to head (6e03b7c).
Report is 59 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14689      +/-   ##
==========================================
+ Coverage   78.95%   78.98%   +0.03%     
==========================================
  Files        1084     1085       +1     
  Lines      146638   147313     +675     
==========================================
+ Hits       115773   116362     +589     
- Misses      23667    23723      +56     
- Partials     7198     7228      +30

Flag	Coverage Δ
unittests	`78.98% <ø> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zyga

I'm very happy to see this. Our CI pipeline was a bit of an evolutionary mess that made it bot slower and harder to understand.

Two notes:

Please double check that the state of the pipeline in master has not moved. As of Friday, 8th of November, there are conflicts suggesting that they did.
Please document workflow inputs so that it's easier to figure out stuff that's less than obvious (like version with the value of "pristine").

Lastly once this lands I'd love to see a follow up to clean up more of this that is now more evident. Things like reused preparation across workflows - local actions handle that very well.

Overall +1 but please resolve conflicts and ping for re-review just prior to merging.

zyga · 2024-11-08T07:51:26Z

.github/workflows/unit-tests.yaml

+      with:
+        # needed for git commit history
+        fetch-depth: 0
+        # NOTE: checkout the code in a fixed location, even for forks, as this


I suspect this is not useful anymore but I think this is something we can explore in another cleanup pass.

How about I add another task for myself after this PR is merged to do some cleanup? Since the file changes make reviewing this PR difficult, I think perhaps it would be better to have only reorganization in this PR and actual changes in another so that it's easy for reviewers to see the changes.

I suppose this is no longer an issue since we added go.mod support. Perhaps it's worth to check if we could simply use the project default location. FWIW, followup material.

zyga · 2024-11-08T07:53:32Z

.github/workflows/unit-tests.yaml

+      if: ${{ inputs.unit-scenario == 'normal' }}
+      run: |
+        cd ${{ github.workspace }}/src/github.com/snapcore/snapd || exit 1
+        ./run-checks --unit


I think this should move to go test ./.... The reason for avoiding vendor directories is AFAIK long gone.

I would propose, as above, that making code changes be part of a follow-up task in cleaning up the actions. That's only because reviewing code across file changes is ugly.

.github/workflows/snap-builds.yaml

zyga · 2024-11-08T07:58:37Z

.github/workflows/static-checks.yaml

+        path: ./src/github.com/snapcore/snapd
+
+    # Fetch base ref, needed for golangci-lint
+    - name: Fetching base ref ${{ github.base_ref }}


All of this typical setup can be broken out further and used across all the workflows. This can be a local action.

After looking through the workflows, I believe the most sensible common point is the "downloading cached Debian dependencies, extracting them, and installing them" stages. I created a local action for those three steps. I didn't see any other obvious groups of steps that are shared between jobs.

I noticed that the unit tests job also fetches the base ref, but I believe that is entirely unnecessary since the unit tests don't run golangci-lint, unless I'm mistaken. So instead of grouping that in with the Debian dependency steps, I would just delete it during the clean-up task.

.github/workflows/spread-tests.yaml

sergiocazzolato

Thanks for this change, I added 2 comments inline.

sergiocazzolato · 2024-11-08T11:49:18Z

.github/workflows/test.yaml

@@ -598,9 +188,20 @@ jobs:
        verbose: true

  spread:
+    uses: ./.github/workflows/spread-tests.yaml
    needs: [unit-tests, snap-builds]
    name: ${{ matrix.group }}


I think we need a better name here, if could be "spread tests (${{ matrix.group }})"

sergiocazzolato · 2024-11-08T11:58:32Z

.github/workflows/test.yaml

@@ -11,7 +11,11 @@ concurrency:

 jobs:
  snap-builds:
-    runs-on: ubuntu-22.04
+    uses: ./.github/workflows/snap-builds.yaml


thinking on keep this workflows directory organize, perhaps we could have a dir .github/workflows/jobs, and store there all the jobs we define.

Unfortunately, github doesn't allow us to have subdirectories:

Creating a reusable workflow

Reusable workflows are YAML-formatted files, very similar to any other workflow file. As with other workflow files, you locate reusable workflows in the .github/workflows directory of a repository. Subdirectories of the workflows directory are not supported.

sergiocazzolato · 2024-11-11T11:58:33Z

The only change I would like to see is the spread executions should be named something like this "spread ubuntu-xxxx"

sergiocazzolato

Excellent work, thanks

zyga

LGTM but have a look at some comments inline and please coordinate with branches landing from @sergiocazzolato

zyga · 2024-11-12T16:58:45Z

.github/workflows/snap-builds.yaml

+        description: 'The go toolchain to use {default, FIPS}'
+        required: true
+        type: string
+      version:


In a follow-up let's rename this to variant or something else. The word version seems confusing as it's not the version string.

zyga · 2024-11-12T17:00:54Z

.github/workflows/spread-tests.yaml

+        description: 'The spread system(s) to use (for possible values, check spread.yaml). If more than one, separate them with a space'
+        required: true
+        type: string
+      tests:


While we use this for testing, technically those are "tasks". Perhaps we could use that naming for consistency.

zyga · 2024-11-12T17:02:02Z

.github/workflows/spread-tests.yaml

+              FAILED_TESTS="$(cat $FAILED_TESTS_FILE)"
+              if [ -n "$FAILED_TESTS" ]; then
+                  echo "Failed tests to run: $FAILED_TESTS"
+                  echo "FAILED_TESTS=$FAILED_TESTS" >> $GITHUB_ENV


Should this not be unconditional? We can just emit an empty value.

I'll take care of this in the cleanup task. Thanks for pointing it out!

zyga · 2024-11-12T17:02:25Z

.github/workflows/spread-tests.yaml

+          if [ -n "$FAILED_TESTS" ]; then
+              RUN_TESTS="$FAILED_TESTS"
+          else
+              for SYSTEM in ${{ inputs.systems }}; do


CC @sergiocazzolato this is changed in the recently-approved PR from you. Please coordinate.

zyga · 2024-11-12T17:03:20Z

.github/workflows/spread-tests.yaml

+        done
+
+    - name: Run spread tests
+      if: "!contains(github.event.pull_request.labels.*.name, 'Skip spread') && ( !startsWith(inputs.group, 'nested-') || contains(github.event.pull_request.labels.*.name, 'Run nested') )"


Ditto about filtering logic.

zyga · 2024-11-12T17:05:17Z

.github/workflows/spread-tests.yaml

+          echo "Running command: $SPREAD $RUN_TESTS"
+          (set -o pipefail; $SPREAD -no-debug-output -logs spread-logs $RUN_TESTS | PYTHONDONTWRITEBYTECODE=1 ./tests/lib/external/snapd-testing-tools/utils/log-filter $FILTER_PARAMS | tee spread.log)
+
+    - name: Uploading spread logs


Suggested change

- name: Uploading spread logs

- name: Upload spread logs

Let's either use continuous or simple tense and stick to it across the jobs/workflows/actions.

zyga · 2024-11-12T17:06:59Z

.github/workflows/static-checks.yaml

+    # golang latest ensures things work on the edge
+    - name: Install the go snap
+      run: |
+          sudo snap install --classic --channel=${{ inputs.gochannel }} go


I wrote https://github.com/canonical/snapd-wsl-tests/tree/main/.github/actions/download-snap and https://github.com/canonical/snapd-wsl-tests/tree/main/.github/actions/install-offline-snap that we might want to use later on. One advantage is that they use GitHub cache so installation is very very fast, even if the store is slow or down completely. They'd have to be ported a little from powershell but the idea could stand.

Nice! I'll look into adding that in the cleanup task. That sounds great.

zyga · 2024-11-12T17:07:36Z

.github/workflows/static-checks.yaml

+              export SKIP_GOFMT=1
+              echo "Formatting checks will be skipped due to the use of Go version ${{ inputs.gochannel }}"
+          fi
+          sudo apt-get install -y python3-yamlordereddictloader


This is such a gotcha.

I'd love to move that part to go at some point.

that would be great

zyga · 2024-11-12T17:08:29Z

.github/workflows/unit-tests-cross-distro.yaml

+        fedora:*)
+            dnf install -y rpmdevtools
+            dnf install -y $(rpmspec -q --buildrequires "./packaging/$distroname/snapd.spec")
+            # TODO these are needed only by cmd/snap-seccomp unit tests, and


Oh, thanks for reminding us about that. Let's do this soonish.

Well, I'm not sure we'll be able to fix it. It's apparently not easy to express such a dependency in the spec last time I attempted to address this

…stall

bboozzoo

Thanks, looks great. I think I would only fix the newlines at this point and leave the rest for a followup PR.

.github/workflows/actions/download-install-debian-deps/action.yaml

.github/workflows/snap-builds.yaml

.github/workflows/spread-tests.yaml

bboozzoo · 2024-11-13T07:07:44Z

.github/workflows/spread-tests.yaml

+          # propagated; and we use a subshell as this option could trigger
+          # undesired changes elsewhere
+          echo "Running command: $SPREAD $RUN_TESTS"
+          (set -o pipefail; $SPREAD -no-debug-output -logs spread-logs $RUN_TESTS | PYTHONDONTWRITEBYTECODE=1 ./tests/lib/external/snapd-testing-tools/utils/log-filter $FILTER_PARAMS | tee spread.log)


since we're already modifying the code:

Suggested change

(set -o pipefail; $SPREAD -no-debug-output -logs spread-logs $RUN_TESTS | PYTHONDONTWRITEBYTECODE=1 ./tests/lib/external/snapd-testing-tools/utils/log-filter $FILTER_PARAMS | tee spread.log)

(

set -o pipefail

$SPREAD -no-debug-output -logs spread-logs $RUN_TESTS | \

PYTHONDONTWRITEBYTECODE=1 ./tests/lib/external/snapd-testing-tools/utils/log-filter $FILTER_PARAMS | \

tee spread.log

)

FWIW, not need to do this now, a followup PR is fine.

Thanks, I'll add this to the clean-up PR to follow

.github/workflows/spread-tests.yaml

.github/workflows/static-checks.yaml

bboozzoo · 2024-11-13T07:14:54Z

.github/workflows/unit-tests-cross-distro.yaml

+        fedora:*)
+            dnf install -y rpmdevtools
+            dnf install -y $(rpmspec -q --buildrequires "./packaging/$distroname/snapd.spec")
+            # TODO these are needed only by cmd/snap-seccomp unit tests, and


Well, I'm not sure we'll be able to fix it. It's apparently not easy to express such a dependency in the spec last time I attempted to address this

.github/workflows/unit-tests-cross-distro.yaml

bboozzoo · 2024-11-13T07:15:52Z

.github/workflows/unit-tests.yaml

+      with:
+        # needed for git commit history
+        fetch-depth: 0
+        # NOTE: checkout the code in a fixed location, even for forks, as this


I suppose this is no longer an issue since we added go.mod support. Perhaps it's worth to check if we could simply use the project default location. FWIW, followup material.

.github/workflows/unit-tests.yaml

maykathm · 2024-11-13T11:34:58Z

The various failures throughout the spread tests are irrelevant to this PR.

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch 2 times, most recently from 589d075 to aa21f6f Compare November 5, 2024 15:08

maykathm requested a review from sergiocazzolato November 5, 2024 17:01

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from aa21f6f to 8c0f1cf Compare November 6, 2024 09:40

bboozzoo requested review from bboozzoo and zyga November 6, 2024 14:08

maykathm marked this pull request as ready for review November 7, 2024 12:41

zyga reviewed Nov 8, 2024

View reviewed changes

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from 8c0f1cf to 2184efa Compare November 8, 2024 09:09

sergiocazzolato reviewed Nov 8, 2024

View reviewed changes

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch 6 times, most recently from d5095ab to 8edc8fd Compare November 8, 2024 15:26

maykathm requested review from sergiocazzolato and zyga November 8, 2024 15:48

zyga mentioned this pull request Nov 8, 2024

tests: Added arm github runner to build snapd #14504

Merged

sergiocazzolato approved these changes Nov 11, 2024

View reviewed changes

zyga approved these changes Nov 12, 2024

View reviewed changes

maykathm added 7 commits November 12, 2024 18:29

tests: moved unit test execution to separate workflow

39241ce

tests: removed snap builds to separate workflow

5d9ae2c

tests: moved static checks to separate workflow

8f478e0

tests: move cross-distro unit tests to separate workflow

9dc13a4

tests: separated spread tests out in separate workflow

62f6e67

tests: added descriptions for workflow_call inputs

51e6191

tests: create composite action from debian cache download and deps in…

c9c4af1

…stall

maykathm added 2 commits November 12, 2024 18:29

tests: change spread test job name

17d3c1c

tests: simplify spread test job name

b3c8074

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from 38bf9df to 5912d51 Compare November 12, 2024 17:31

tests: renamed parameters and stage

c6bf903

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from 5912d51 to c6bf903 Compare November 12, 2024 17:32

bboozzoo approved these changes Nov 13, 2024

View reviewed changes

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from 2035734 to 519a3a1 Compare November 13, 2024 08:11

tests: added missing newlines

6e03b7c

maykathm force-pushed the SNAPDENG-34130-split-workflow-into-smaller-parts branch from 519a3a1 to 6e03b7c Compare November 13, 2024 08:55

sergiocazzolato merged commit 9a71592 into canonical:master Nov 13, 2024
49 of 57 checks passed

maykathm deleted the SNAPDENG-34130-split-workflow-into-smaller-parts branch November 13, 2024 15:41

maykathm mentioned this pull request Nov 20, 2024

tests: cleanup github workflows #14745

Merged

tests: split workflow into smaller parts #14689

tests: split workflow into smaller parts #14689

Conversation

maykathm commented Nov 5, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

zyga left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maykathm Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

sergiocazzolato left a 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

Creating a reusable workflow

sergiocazzolato commented Nov 11, 2024

sergiocazzolato left a comment

Choose a reason for hiding this comment

zyga left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a 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

Choose a reason for hiding this comment

maykathm commented Nov 13, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

maykathm Nov 8, 2024 •

edited

Loading