Skip to content

Add contrib/subtree test execution to CI builds #3349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Add contrib/subtree test execution to CI builds #3349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20fdb2e
subtree: update `contrib/subtree` `test` target
vdye Aug 5, 2021
aceb7a0
ci: run `contrib/subtree` tests in CI builds
vdye Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ci/run-build-and-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,21 @@ linux-gcc)
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
export GIT_TEST_WRITE_REV_INDEX=1
export GIT_TEST_CHECKOUT_WORKERS=2
make test
make test && make -C contrib/subtree test || exit 1
;;
linux-clang)
export GIT_TEST_DEFAULT_HASH=sha1
make test
export GIT_TEST_DEFAULT_HASH=sha256
make test
make test && make -C contrib/subtree test || exit 1
;;
linux-gcc-4.8)
# Don't run the tests; we only care about whether Git can be
# built with GCC 4.8, as it errors out on some undesired (C99)
# constructs that newer compilers seem to quietly accept.
;;
*)
make test
make test && make -C contrib/subtree test || exit 1
;;
esac

Expand Down
3 changes: 3 additions & 0 deletions ci/run-test-slice.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@ make --quiet -C t T="$(cd t &&
./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh |
tr '\n' ' ')"

# Run the git subtree tests only if main tests succeeded
test 0 != "$1" || make -C contrib/subtree test

check_unignored_build_artifacts
2 changes: 1 addition & 1 deletion contrib/subtree/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ $(GIT_SUBTREE_TEST): $(GIT_SUBTREE)
cp $< $@

test: $(GIT_SUBTREE_TEST)
$(MAKE) -C t/ test
$(MAKE) -C t/ all

Comment on lines 96 to 98
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

clean:
$(RM) $(GIT_SUBTREE)
Expand Down