Unit test execution is not part of the PR list of checks

[snebel29]

Hi,
During works on requested changes to an existing PR, I noticed all the checks in the PR (DCO, kubernetes tests and linter) were green, while after doing a simple go test ./... in the root of the project showed one test case and one pre-check were failing.

I wonder, since its execution is relatively cheap is there a reason not to include them earlier? also are they executed automatically later before allowing to merge?

Thanks.

[miekg]

doing a go test your self seems like a prudent way to do anyway

[snebel29]

Of course, I was only thinking that with little effort an extra check could be added that also run the unit tests and disable merging is is red. This is not a game changer but may be handy, If someone else see some value I can take a look.

[chrisohaver]

My 2 cents: I'd prefer not to disable merging for failed tests. This could complicate situations where we have changes which are dependent on changes in tests in other repos - specifically with the coredns/ci tests.

Maybe just a gentle reminder to folks with merge privilege to take care when merging, e.g. Wait for tests complete before merging, and any failures are flakes, or expected transient failures. Don't merge anything that you have not personally reviewed the changes for (even if someone else has approved).

[snebel29]

My 2 cents: I'd prefer not to disable merging for failed tests. This could complicate situations where we have changes which are dependent on changes in tests in other repos - specifically with the coredns/ci tests.

OK, In my mind still you could always force merge or something in those lines but didn't dig on the repo config. It just felt handy to me as a reminder to have an extra check with the execution result of unit test (not just the integration test ones in coredns/ci) and a common practice out there.

I'm definitively not pushing to do this :) I'm happy to keep the status quo.