simplify integration testing with matrix jobs #1799

vdovhanych · 2024-02-29T14:07:29Z

This PR significantly reduces the number of integration test workflow files. I modified the setup to use the GitHub matrix strategy instead of having 90+ generated workflow files.

I also tried to keep the original go script that generated the workflow files, but now, instead of generating whole workflow files, it adds test names to a matrix strategy. This, in turn, improves readability and updates to the test structure.

This will spawn a job for every combination, so if we fe. have TestACLHostsInNetMapTable and database: [postgres, sqlite] for the matrix strategy, this will create two jobs

(TestACLHostsInNetMapTable, postgres)
(TestACLHostsInNetMapTable, sqlite)

Or see how it will look in the PR for the already run tests.

Caveats to this setup:

requires you to have yq installed so you can generate test names for the workflow
yq will remove the empty line formatting from the GitHub file, so either fix it manually or commit it with those changes (it does not affect the functionality, but it's not as convenient to read as the current formatting)
the naming of the test is not that clear but can be improved to reflect better what it does( especially in regards to 0 or 1 for the usage of postgres)

I have checked that the failing tests are not connected to this change. The same issues are currently present with the previous setup.

read the CONTRIBUTING guidelines
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

kradalby · 2024-02-29T14:12:07Z

cmd/gh-action-integration-generator/main.go

-				name = test + "-postgres"
-			}
+	yqCommand := fmt.Sprintf(
+		"yq eval '.jobs.integration-test.strategy.matrix.test = %s' ../../.github/workflows/test-integration.yaml -i",


can you add yq to the flake.nix file so its automatically installed for the dev env? that way we should not run into an issue with needing it.

Sure will add it there

Little sidenote, is there any particular reason why gofumpt is not part of the devdeps in the nix shel?

No it should be, I must have missed it, that can be done in a followup.

Currently we are lacking a bit on jobs to verify the formatting, so a job similar to the "is the tests generated" would potentially be beneficial for golines + gofumpt.

kradalby · 2024-02-29T14:12:58Z

.github/workflows/test-integration.yaml

+          - TestSSHNoSSHConfigured
+          - TestSSHIsBlockedInACL
+          - TestSSHUserOnlyIsolation
+        postgres: [0, 1]


Would be neat if we could fix this naming right of the bat with -sqlite and -postgres

Yeah ill try doing it like this

database: [postgres, sqlite]

and use the if commands for the variable so if postgres=1 if sqlite postgres=0

But maybe a better solution would be to change what code expects to get from the env.

Ok i changed the workflow it will have the name -postgres or -sqlite and i change it to if for the env itself if its 1 or 0 :)

kradalby · 2024-02-29T14:13:32Z

In general looks good, thanks for replacing my hacks with some other neater hacks 👍

vdovhanych · 2024-02-29T14:49:57Z

Sorry for one more force push, I realized that the yq I'm using is yq-go in the nix packages.

kradalby · 2024-03-01T07:08:33Z

hmm, thats some weird postgres failure, all at the same time.

kradalby · 2024-03-01T07:44:43Z

Fix in #1802

kradalby · 2024-03-01T07:47:51Z

I'm just throwing this out there since you have made a very nice change, It would be really nice to have a workflow that runs the generate command and if there is a new file that isnt checked in, it fails so people (me mostly) do not forget to add the new tests.

If you are interested in helping more, feel free to see if there are other CI stuff that can be improved, it is mostly something I do the shortest path to success because I really want to spend time on other things, as you can see with the original implementation. The only thing I ask is that its done with Nix so I can have it replicated locally.

vdovhanych · 2024-03-01T08:44:49Z

I'll look at what could be improved over the weekend.

About the workflow, you're talking about it. Do you mean it will check what tests are in the workflow test-integration.yaml, compare it with the tests folder, and fail if not all of the tests are in that workflow?

Happy to help with this project and relieve some of your time towards the features 😄

kradalby · 2024-03-01T11:02:00Z

I'll look at what could be improved over the weekend.

Thank you!

About the workflow, you're talking about it. Do you mean it will check what tests are in the workflow test-integration.yaml, compare it with the tests folder, and fail if not all of the tests are in that workflow?

I think the easiest way would be to do something like:

git checkout
cd cmd/gh-generator
go generate
git status <-- fail if there are any new files

Happy to help with this project and relieve some of your time towards the features 😄

Thank you, I would say this is in the category of "we need it" but not "core business" 😄

vdovhanych · 2024-03-01T12:13:08Z

I added the check you suggested. ✔️
Also, if you look at the test-integration workflow, it is now formatted stupidly, but it is a limitation of yq, and for the check to work, we need to go with how the yq formats it, so we are left with the diff that's actually relevant, for the use case.

kradalby · 2024-03-01T18:14:04Z

Great! Ppstfix is in so a rebase should make it happier

kradalby · 2024-03-02T00:38:52Z

.github/workflows/check-tests.yaml

+      - name: Generate and check integration tests
+        if: steps.changed-files.outputs.any_changed == 'true'
+        run: |
+          nix develop --run "cd cmd/gh-action-integration-generator && go generate"


I wonder if this needs to be:

nix develop --run -- "cd cmd/gh-action-integration-generator && go generate"

noo its supposed to be nix develop --command I'm used to nix-shell where its --run sorry will fix right away

vdovhanych · 2024-03-02T09:07:12Z

Just noticed, maybe go ahaed and cancell the previous waiting checks for aproovement as it now cancells these latest ones and its now running not the final setup.

It got stuck in some loop of cancelling a reruning the checks, getting tons of notifications from github 😅

vdovhanych · 2024-03-02T12:38:06Z

I dont know what is going on, it keeps spamming me with the notifications i think i managed to confuse GitHub 😆

can you try cancelling all of the workflows here and here and then try to rerun the very latest ones?

…kflow

vdovhanych requested review from juanfont and kradalby as code owners February 29, 2024 14:07

kradalby reviewed Feb 29, 2024

View reviewed changes

vdovhanych force-pushed the github-workflows-improvements branch 2 times, most recently from d2fb5aa to ae6fad4 Compare February 29, 2024 14:48

vdovhanych force-pushed the github-workflows-improvements branch from ae6fad4 to 8c7aa96 Compare March 1, 2024 12:07

vdovhanych force-pushed the github-workflows-improvements branch from 8c7aa96 to 71cdb61 Compare March 1, 2024 21:21

kradalby reviewed Mar 2, 2024

View reviewed changes

vdovhanych force-pushed the github-workflows-improvements branch from 71cdb61 to cddce3a Compare March 2, 2024 08:28

simplify integration testing with matrix jobs

c15d6a6

vdovhanych force-pushed the github-workflows-improvements branch from cddce3a to 30545e9 Compare March 2, 2024 12:29

vdovhanych requested a review from kradalby March 2, 2024 12:33

check if all of the integration tests are in the test-integration wor…

90d7b8a

…kflow

vdovhanych force-pushed the github-workflows-improvements branch from 30545e9 to 90d7b8a Compare March 2, 2024 20:15

kradalby approved these changes Mar 2, 2024

View reviewed changes

kradalby merged commit e15a083 into juanfont:main Mar 2, 2024
93 of 98 checks passed

vdovhanych mentioned this pull request Mar 3, 2024

workflows naming convention vdovhanych/headscale#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify integration testing with matrix jobs #1799

simplify integration testing with matrix jobs #1799

vdovhanych commented Feb 29, 2024 •

edited

Loading

kradalby Feb 29, 2024

vdovhanych Feb 29, 2024

vdovhanych Feb 29, 2024

vdovhanych Feb 29, 2024

kradalby Mar 2, 2024

kradalby Feb 29, 2024

vdovhanych Feb 29, 2024

vdovhanych Feb 29, 2024

kradalby commented Feb 29, 2024

vdovhanych commented Feb 29, 2024

kradalby commented Mar 1, 2024

kradalby commented Mar 1, 2024

kradalby commented Mar 1, 2024

vdovhanych commented Mar 1, 2024

kradalby commented Mar 1, 2024

vdovhanych commented Mar 1, 2024

kradalby commented Mar 1, 2024

kradalby Mar 2, 2024

vdovhanych Mar 2, 2024

vdovhanych Mar 2, 2024

vdovhanych commented Mar 2, 2024 •

edited

Loading

vdovhanych commented Mar 2, 2024 •

edited

Loading

simplify integration testing with matrix jobs #1799

simplify integration testing with matrix jobs #1799

Conversation

vdovhanych commented Feb 29, 2024 • edited Loading

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

kradalby commented Feb 29, 2024

vdovhanych commented Feb 29, 2024

kradalby commented Mar 1, 2024

kradalby commented Mar 1, 2024

kradalby commented Mar 1, 2024

vdovhanych commented Mar 1, 2024

kradalby commented Mar 1, 2024

vdovhanych commented Mar 1, 2024

kradalby commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdovhanych commented Mar 2, 2024 • edited Loading

vdovhanych commented Mar 2, 2024 • edited Loading

vdovhanych commented Feb 29, 2024 •

edited

Loading

vdovhanych commented Mar 2, 2024 •

edited

Loading

vdovhanych commented Mar 2, 2024 •

edited

Loading