Make test on every commit #3789

christoph-zededa · 2024-03-01T18:26:35Z

Run make test on every commit of a PR.

Advantages:

Every commit (in pillar) builds and the tests succeed and this helps when bisecting
More likely to find flaky tests

Disadvantages:

It slows down building the PR (roughly 20 minutes with average amount of commits)
Perhaps it is too strict and scares people away from doing a PR or slows them down
It punishes splitting a PR in more commits

codecov · 2024-03-01T19:07:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (2c42bfc) to head (a1adf81).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3789   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35

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

europaul · 2024-03-04T09:08:39Z

@christoph-zededa just to be sure: we already execute unit tests on every PR, but here you are proposing to execute them not just on every PR, but on every commit in the PR, right?

rouming · 2024-03-04T09:30:06Z

Would be great to have this optional, so that everyone can decide does it make sense to run the whole test suite against each commit. This measure if it is a requirement looks more like a punishment for me. Testing is good, no doubts, but this is too much.

christoph-zededa · 2024-03-04T10:06:36Z

@christoph-zededa just to be sure: we already execute unit tests on every PR, but here you are proposing to execute them not just on every PR, but on every commit in the PR, right?

Yes.

christoph-zededa · 2024-03-04T10:08:24Z

Would be great to have this optional, so that everyone can decide does it make sense to run the whole test suite against each commit. This measure if it is a requirement looks more like a punishment for me. Testing is good, no doubts, but this is too much.

It is already optional; (nearly) everybody can just run the same command on their machine.

P.S.: Waiting for somebody to do the same for yetus ...

europaul · 2024-03-04T10:18:37Z

Makefile

@@ -396,6 +396,10 @@ test: $(LINUXKIT) test-images-patches | $(DIST)
 	$(QUIET)$(DOCKER_GO) "cd \"$(GOTREE)\"; ../../tools/fuzz_test.sh" $(GOTREE) $(GOMODULE)
 	$(QUIET): $@: Succeeded

+test-commits:
+	GIT_SEQUENCE_EDITOR=/bin/true git rebase -i --exec 'make test' origin/master


wow, this is a nice git command! didn't know something like this was possible 👍

europaul

since unit tests already run on every push to any PR, I think most commits are already covered this way. I would leave it as is

rene · 2024-03-04T10:43:22Z

I think for each commit it's a

since unit tests already run on every push to any PR, I think most commits are already covered this way. I would leave it as is

I agree with @europaul . Also, I think it's nice to have the target test-commits in the Makefile (@christoph-zededa , pls, add the corresponding documentation for make help), but I wouldn't add to the GH actions, our resources are still limited and I'm afraid that by enforcing this running for every single commit can slow down too much the PR reviewing process...

christoph-zededa · 2024-03-04T11:02:20Z

I think for each commit it's a

since unit tests already run on every push to any PR, I think most commits are already covered this way. I would leave it as is

I agree with @europaul . Also, I think it's nice to have the target test-commits in the Makefile (@christoph-zededa , pls, add the corresponding documentation for make help), but I wouldn't add to the GH actions, our resources are still limited and I'm afraid that by enforcing this running for every single commit can slow down too much the PR reviewing process...

Actually I don't think it is a good idea to have it in the Makefile; I would rather put the command directly into the github action.
Running make test-commits confuses the user if they don't know what is happening beneath it; f.e. "Why does running it get me into a rebase conflict?"

If you want to run it, you can just call git yourself.

uncleDecart · 2024-03-05T11:01:21Z

pkg/pillar/cmd/zedrouter/patchenvelopes_test.go

@@ -20,6 +20,7 @@ import (
 // TestPatchEnvelopes creates PatchEnvelope then deletes
 // one of them and checks of it was properly deleted
 func TestPatchEnvelopes(t *testing.T) {
+	t.Skip("this test sporadically fails")


See #3792 I'm checking how it is running in runner, locally I ran it 300 times and didn't have any problems

uncleDecart · 2024-03-05T11:11:05Z

I think it makes sense to have it locally also, because before publishing one can check if their commits don't break unit tests. Introducing it into CI/CD won't affect costs much, but I'd gather more data. Back of envelope calculations are like: on average it takes ~4 mins to run unit test on average we have ~5 commits per PR so it's additional 20 mins per PR. With Buildjet runners priced for 0.008$ per minute, it'll result in 0.16$ per PR on average. If we have around 60 PR per month (two each day) that will result in 9.6$ additional cost, I think we can live with that.

As for process: we can always run it in parallel to other workflows :)

christoph-zededa · 2024-03-05T13:35:29Z

I think it makes sense to have it locally also, because before publishing one can check if their commits don't break unit tests. Introducing it into CI/CD won't affect costs much, but I'd gather more data. Back of envelope calculations are like: on average it takes ~4 mins to run unit test on average we have ~5 commits per PR so it's additional 20 mins per PR. With Buildjet runners priced for 0.008$ per minute, it'll result in 0.16$ per PR on average. If we have around 60 PR per month (two each day) that will result in 9.6$ additional cost, I think we can live with that.

As for process: we can always run it in parallel to other workflows :)

Thank you very much for adding those numbers!

eriknordmark · 2024-03-07T08:25:07Z

P.S.: Waiting for somebody to do the same for yetus ...

FWIW we used to have a working 'make yetus' but it has fallen into disrepair.

eriknordmark · 2024-03-07T08:28:19Z

If we have around 60 PR per month (two each day) that will result in 9.6$ additional cost, I think we can live with that.

@uncleDecart I'm pretty sure each PR goes through multiple builds (updates to fix yetus issues, code review comments, rebase on master, etc). It is easy to extract how many e.g., yetus or unit test workflows we've run in a month so we can compare with how many PRs we did during that month?

rouming · 2024-03-18T12:12:08Z

If I read everyone right we want to keep this in Makefile, but not to make this as a part of GH actions. @christoph-zededa can you please change this PR accordingly so we can merge?

this new target runs go tests on every commit of a branch (relative to the master branch) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa · 2024-03-18T13:22:03Z

If I read everyone right we want to keep this in Makefile, but not to make this as a part of GH actions. @christoph-zededa can you please change this PR accordingly so we can merge?

done - please have a look at the change in the Makefile

rouming

Ack from me.

rouming · 2024-04-05T10:24:46Z

@christoph-zededa Please add the help line into the Makefile as well and let's merge this

christoph-zededa · 2024-04-15T12:22:01Z

Closing as it seems nobody needs this
and nobody wants to have it on CI.

github-actions bot requested review from eriknordmark, milan-zededa and rouming March 1, 2024 18:27

christoph-zededa force-pushed the make_test_on_every_commit branch 5 times, most recently from 5a72c4a to 8e6d93c Compare March 1, 2024 18:47

europaul reviewed Mar 4, 2024

View reviewed changes

uncleDecart reviewed Mar 5, 2024

View reviewed changes

Makefile: test-commits

a1adf81

this new target runs go tests on every commit of a branch (relative to the master branch) Signed-off-by: Christoph Ostarek <christoph@zededa.com>

christoph-zededa force-pushed the make_test_on_every_commit branch from 8e6d93c to a1adf81 Compare March 18, 2024 13:20

rouming approved these changes Apr 5, 2024

View reviewed changes

rouming changed the title ~~RFC: Make test on every commit~~ Make test on every commit Apr 5, 2024

rouming marked this pull request as ready for review April 5, 2024 10:23

christoph-zededa closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make test on every commit #3789

Make test on every commit #3789

christoph-zededa commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

europaul commented Mar 4, 2024

rouming commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

europaul Mar 4, 2024

europaul left a comment

rene commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

uncleDecart Mar 5, 2024

uncleDecart commented Mar 5, 2024

christoph-zededa commented Mar 5, 2024

eriknordmark commented Mar 7, 2024

eriknordmark commented Mar 7, 2024

rouming commented Mar 18, 2024

christoph-zededa commented Mar 18, 2024

rouming left a comment

rouming commented Apr 5, 2024

christoph-zededa commented Apr 15, 2024

Make test on every commit #3789

Make test on every commit #3789

Conversation

christoph-zededa commented Mar 1, 2024 • edited Loading

codecov bot commented Mar 1, 2024 • edited Loading

Codecov Report

europaul commented Mar 4, 2024

rouming commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

europaul Mar 4, 2024

Choose a reason for hiding this comment

europaul left a comment

Choose a reason for hiding this comment

rene commented Mar 4, 2024

christoph-zededa commented Mar 4, 2024

uncleDecart Mar 5, 2024

Choose a reason for hiding this comment

uncleDecart commented Mar 5, 2024

christoph-zededa commented Mar 5, 2024

eriknordmark commented Mar 7, 2024

eriknordmark commented Mar 7, 2024

rouming commented Mar 18, 2024

christoph-zededa commented Mar 18, 2024

rouming left a comment

Choose a reason for hiding this comment

rouming commented Apr 5, 2024

christoph-zededa commented Apr 15, 2024

christoph-zededa commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading