Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push

Hi Emily,

On Wed, 3 Jul 2019, Emily Shaffer wrote:

If I were tasked with developing this further, I would try to move as much of the setup into the initial test case (if there is already a `setup` test case; otherwise I would create one). In fact, I would try to reuse as much of the existing setup test case as possible, and only add commands if really necessary. Then, I would try to combine the three test cases in the patch into a single one, structuring it by white space and using comments to clarify what is happening.

In my mind, even just adding an empty line before the comments like "Make master incompatible with up/master" would make it much easier for me to read, were I to analyze a test breakage.

I have to admit that I have a hard time understanding what the intention of those three test cases is because I get confused: where does the set-up end, where is the code that is expected to fail, where are the expectations tested?

Also, I get confused by how similar the test cases look, and have a hard time discerning what the differences are (i.e. what are the interesting bits, where the entropy comes from).

I could imagine that I would have had an easier time reading something like this:

test_expect_success 'push --atomic' '
	: set up two branches, one which will require a force push &&
	git checkout -b fast-forwarding master &&
	test_commit push-atomic &&
	git checkout -b non-fast-forwarding &&

	: now, initialize the bare repository to push to &&
	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&
	git clone --bare . $d &&

	: modify the two branches and create a third one &&
	git reset --hard HEAD^ &&
	git checkout fast-forwarding &&
	test_commit no-need-to-force &&
	git branch new-branch &&

	: now the atomic push must fail &&
	test_must_fail git push --atomic "$HTTPD_URL"/smart/atomic.git \
		fast-forwarding non-fast-forwarding new-branch 2>err &&

	: verify that new-branch was not pushed &&
	test_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&

	: fast-forwarding should be mentioned even if it would have been OK &&
	grep fast-forwarding err
'

Of course, everybody has their preferences, and their personal style. I do not want you to imitate my style just to "pacify" me. That's not the point of this example.

The point is that I need some structure to walk along, especially when I am a bit annoyed at a test case that shows that I introduced a regression and all I want is to understand as quickly as possible what I did wrong again so that I can fix it and move on.

The point is that I want a regression test case to _not_ distract me from the essential part, ideally I should be able to ignore all the set-up code and deduce from the command-line of the failing command what is going on.

For example, if the test case fails in the line `grep fast-forwarding err` above, that command-line alone does not tell me anything, it just frustrates me. If there is a comment above that line (which is ideally part of the `-x` trace, that's why I used `:` instead of `#`) that states the intention in what I consider to be clear, simple English, it is a lot easier to figure out what the heck is going wrong.

Of course, it would be even better if we had a function, say, `must_contain` that runs that `grep` and shows the file contents and the regular expression if that `grep` failed. That's of course outside of the concern of your patch. But this idea illustrates what I want in a regression test case: I want it to _help_ me figure out things when it breaks.

Ciao, Dscho

P.S.: Please note that the many test cases _I_ introduced into Git's test suite mostly do not conform to what I wrote above. I learned _quite_ a lot of how I want regression test cases to look like in the past six months, not from writing them, but from analyzing literally hundreds of Azure Pipelines builds. And your question forced me to think about my learnings to formulate the above (hopefully clear) explanation of what I want in a regression test, so: thank you.