RFC: Make test on every commit

[christoph-zededa]

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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[europaul]

@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]

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]

@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]

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 ...

[rene]

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]

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]

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]

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]

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]

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]

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?

[christoph-zededa]

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]

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

[christoph-zededa]

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