From: "Kyle J. McKay" <mackyle@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git mailing list <git@vger.kernel.org>,
Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs
Date: Sat, 30 Jan 2021 16:26:55 -0700 [thread overview]
Message-ID: <81AEEED6-26EC-4F32-AA65-B028435D812D@gmail.com> (raw)
In-Reply-To: <xmqqa6sqp827.fsf@gitster.c.googlers.com>
On Jan 30, 2021, at 11:56, Junio C Hamano wrote:
>> Add a new set of tests to exercise and demonstrate this change
>> in preparation for correcting it (at which point the failing tests
>> will be flipped from `test_expect_failure` to `test_expect_success`).
>
> We usually prefer not to do the two-step "expect_failure first and
> then in a separate patch flip that to _success", as it makes it hard
> to see the "fix" step (because the change in the test would be shown
> only 3 lines before and after _failure->_success line, and what the
> test is doing cannot be seen in the patch by itself).
I'm having a bit of trouble parsing that into expectations. A little
help please.
Given the scenario that a bug is found that is not being caught by a
test (irrespective of whether or not the outcome of this particular
series discussion results in it being determined to be a "bug").
Further, if the fix is simple enough that it warrants only a single
patch, what is the preferred order of patches then?
I would like the patch series to demonstrate:
1) that the test actually catches the bug (in case it comes back in
the future)
2) the fix isolated (as much as possible) in a patch distinct from the
test
3) that the test now passes, preferably with minimal changes to be
sure that it hasn't accidentally been rendered ineffective
All the while, at no point after commits for (1), (2), or (3) is the
test suite allowed to generate extra failures (that are not marked
"expect failure").
patch 1/2 accomplishes (1)
patch 2/2 does both (2) and (3)
Are you suggesting that (1) just be omitted? Or that it be modified
so that it's an "expect success" patch? But then (2) would break it
and introduce a failing test into the test suite. Should (2) then
just flip the test from test_expect_success to test_expect_failure and
then a separate commit flip it back to test_expect_success along with
the minor change to the test itself to make it start passing again?
In that case, there's always a risk that changing the test itself
could accidentally make it no longer detect the problem anymore and
just always succeed even if the problem comes back.
Without an initial expect_failure step (1), there's no commit in the
repository that can demonstrate that the test actually catches the
problem.
I understand that when new code is added, the tests come after, but it
seems to me that when fixing a pre-existing problem, the test that
demonstrates the problem should come first and be an expect failure
since it is considered to be a problem.
What exactly is the order of test/fix changes that you're expecting to
see here?
Thanks.
next prev parent reply other threads:[~2021-01-30 23:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-30 10:19 [PATCH 0/2] Eliminate extraneous ref log entries Kyle J. McKay
2021-01-30 10:19 ` [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs Kyle J. McKay
2021-01-30 18:56 ` Junio C Hamano
2021-01-30 23:02 ` Kyle J. McKay
2021-01-30 23:48 ` Junio C Hamano
2021-02-01 11:09 ` Han-Wen Nienhuys
2021-01-30 23:26 ` Kyle J. McKay [this message]
2021-01-31 0:11 ` Junio C Hamano
2021-01-30 10:19 ` [PATCH 2/2] refs.c: avoid creating extra unwanted reflog entries Kyle J. McKay
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=81AEEED6-26EC-4F32-AA65-B028435D812D@gmail.com \
--to=mackyle@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hanwen@google.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).