Add contrib/subtree test execution to CI builds

[vdye]

Changes

• Subtree tests run if (and only if) make test passes in Windows, Linux, and Mac build/test jobs
  • For platforms that run make test more than once in run-build-and-tests.sh, subtree tests are only run after the last execution of make test
  • Added "fail fast" exit 1 to run-build-and-tests.sh - otherwise, tests failing in make test wouldn't always fail the test execution (this might also be because I'm not great at bash scripting 😉)
• contrib/subtree test target updated to invoke t/ directory all target (rather than test target)
  • Necessary to use the $DEFAULT_TEST_TARGET
  • Matches root test target behavior

Testing

• CI execution with no failing tests is linked to this PR
• CI execution with forced failure in t0000: https://github.com/vdye/git/actions/runs/1102256177
  • Skips git subtree tests
  • Fails all test executions
• CI execution with forced failure in subtree tests: https://github.com/vdye/git/actions/runs/1102312905
  • git subtree tests executed only after make test passes
  • Fails all test executions (at least, those that run the subtree tests)

[derrickstolee]

Is this just a bug in the existing makefile? Did make -C contrib/subtree test just not work before?

This might be a good one-liner to send upstream, even if we don't choose to do the rest of the change. I would split them into two commits, first this change, then the changes to ci/.

[dscho]

I think the idea here is that all defaults to running DEFAULT_TEST_TARGET, which is set to prove in the CI runs. And test is a different target.

An alternative I briefly considered was to call make twice: once to "build" git-subtree, and then the test run. But that would be rather ugly because the tests require the git-subtree file to be copied to ../.., which the test target does, thanks to the prerequisite $(GIT_SUBTREE_TEST).

In short: I concur with @derrickstolee that it would be best to split out the change to the Makefile, and the commit message should probably mention that we're aligning with how Git's top-level Makefile defines its own test target (it also calls $(MAKE) -C t/ all, and then also mention that we want to be able to call this make target in contrib/subtree in the CI runs, where DEFAULT_TEST_TARGET is defined as prove.

[vdye]

it would be best to split out the change to the Makefile

Sounds good! Depending on whether you'd like me to make the change or not, should changing the use of pre-clean be part of the same commit, or a separate one?

[dscho]

It can definitely be not only a separate commit, but an independent patch series. And nothing about that patch series would be urgent: since we lost Azure Pipelines' powerful analysis/statistics/UI for tests (see e.g. here), we do not actually care about the full test results to be maintained. We are only interested in them if anything fails. And since we only run the subtree tests if Git's own test suite passed, that desire is still addressed.

However, should we ever decide to always run the subtree tests (even if Git's test suite failed, in which we obviously would still want to fail the test, i.e. record the exit status indicating failure in some way), we would definitely need to get rid of the automatic pre-clean dependency.

For now, though, there is no point in holding this PR up, I think. It would be good to have it in -rc2 (I would probably reorder the picks and the merge somewhat, so that the Merge 'readme' commit still comes on top).

[dscho]

@vdye could you please also describe in the commit message that we are so interested in subtree because it is included in Git for Windows (as opposed to, say, pretty much everything else in contrib)?

[dscho]

Oh, and I think there is yet another snag: contrib/subtree/t/Makefile contains that pre-clean target that is apparently called as part of the prove target, and it completely wipes away the results from Git's regular regression tests... see https://github.com/git-for-windows/git/pull/3349/checks?check_run_id=3255131320#step:5:175

I am not sure that I like the sound of this. Granted, we currently call make test && make -C contrib/subtree test which means that we only run the subtree tests if the regular regression tests all passed, so we don't delete any logs of any failed tests here. But it feels icky nevertheless. Maybe we should simply do away with the pre-clean rule?

[vdye]

that pre-clean target that is apparently called as part of the prove target, and it completely wipes away the results from Git's regular regression tests

I wanted to avoid making changes that caused contrib/subtree to differ in functionality from the main t/ directory test target (since they seem intentionally mostly the same).

Maybe we should simply do away with the pre-clean rule?

If we did remove the pre-clean target, should it only be removed for prove? That would get us what we need for CI testing, while leaving the test target the same. In any case (removing it from prove and/or test), would you want it removed from just contrib/subtree/t/Makefile, or also from t/Makefile & t/perf/Makefile (and, downstream, contrib/scalar/t/Makefile)?

[dscho]

All my outstanding suggestions can definitely wait for their own patch series in the future. This is good to go!

[dscho]

I wanted to avoid making changes that caused contrib/subtree to differ in functionality from the main t/ directory test target (since they seem intentionally mostly the same).

That makes sense, I just have a hunch that upstream might be amenable to adjust this so that contrib/subtree/ never removes the test results.

Maybe we should simply do away with the pre-clean rule?

If we did remove the pre-clean target, should it only be removed for prove? That would get us what we need for CI testing, while leaving the test target the same. In any case (removing it from prove and/or test), would you want it removed from just contrib/subtree/t/Makefile, or also from t/Makefile & t/perf/Makefile (and, downstream, contrib/scalar/t/Makefile)?

My favorite solution would be to keep it as a stand-alone target, but remove it as a dependency from all other rules. The point being: if you want to run the tests as part of CI, chances are that you already ran Git's own CI and want to preserve the test results.

But as I said, this can wait for another time.