From: Emily Shaffer <emilyshaffer@google.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
Date: Tue, 9 Jul 2019 13:23:55 -0700 [thread overview]
Message-ID: <20190709202355.GA113966@google.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1907040947170.44@tvgsbejvaqbjf.bet>
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
next prev parent reply other threads:[~2019-07-09 20:24 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-02 0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
2019-07-02 13:51 ` Johannes Schindelin
2019-07-02 18:27 ` Junio C Hamano
2019-07-03 18:56 ` Emily Shaffer
2019-07-03 19:01 ` Emily Shaffer
2019-07-03 19:41 ` Johannes Schindelin
2019-07-03 20:57 ` Emily Shaffer
2019-07-04 8:29 ` Johannes Schindelin
2019-07-09 20:23 ` Emily Shaffer [this message]
2019-07-02 19:06 ` Junio C Hamano
2019-07-02 20:16 ` Junio C Hamano
2019-07-03 0:09 ` Emily Shaffer
2019-07-02 21:37 ` Junio C Hamano
2019-07-03 0:08 ` Emily Shaffer
2019-07-03 9:10 ` SZEDER Gábor
2019-07-03 18:13 ` Junio C Hamano
2019-07-03 18:58 ` Emily Shaffer
2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
2019-07-10 17:44 ` Junio C Hamano
2019-07-10 17:53 ` Junio C Hamano
2019-07-11 21:14 ` Emily Shaffer
2019-07-11 20:57 ` Emily Shaffer
2019-07-11 21:13 ` Junio C Hamano
2019-07-11 21:19 ` [PATCH v3] " Emily Shaffer
2019-07-12 16:25 ` Junio C Hamano
2019-07-16 7:10 ` [PATCH v2] " Carlo Arenas
2019-07-16 16:53 ` Junio C Hamano
2019-07-16 17:21 ` [RFC/PATCH] CodingGuidelines: spell out post-C89 rules Junio C Hamano
2019-07-17 0:55 ` Jonathan Nieder
2019-07-17 16:03 ` Junio C Hamano
2019-07-19 1:15 ` Jonathan Nieder
2019-07-17 1:09 ` Bryan Turner
2019-07-17 16:05 ` Junio C Hamano
2019-07-16 18:00 ` [PATCH v2] transport-helper: enforce atomic in push_refs_with_push Carlo Arenas
2019-07-16 20:28 ` [PATCH] transport-helper: avoid var decl in for () loop control Junio C Hamano
2019-07-17 0:42 ` Jonathan Nieder
2019-07-18 15:22 ` [PATCH v2] transport-helper: enforce atomic in push_refs_with_push SZEDER Gábor
2019-07-18 16:12 ` Junio C Hamano
2019-07-18 23:46 ` SZEDER Gábor
2019-07-18 16:29 ` Eric Sunshine
2019-07-19 1:31 ` Jonathan Nieder
2019-07-19 4:49 ` Carlo Arenas
2019-07-19 19:15 ` Junio C Hamano
2019-07-27 8:43 ` SZEDER Gábor
2019-07-27 16:11 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190709202355.GA113966@google.com \
--to=emilyshaffer@google.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).