Firstly, sorry for the delay, I wasn't working for national holiday from the 4th til yesterday. > 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 > ' After I read your initial comment this is close to what my rewrite started to look like; I suppose we're only the same page after all :) > > 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. > This writeup is great, and I think your example is very clear and readable. I'll push a reroll with my similar-looking reformat soon. Thanks, Johannes. - Emily
