Re: [PATCH 1/1] t3301: fix false negative
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 9, 2019, 11:33 UTC
- Message-ID
- <xmqq4l775s7d.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <a01a8c704ba03213aa59c59384dba46502089522.1554806481.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In 6956f858f6 (notes: implement helpers needed for note copying during > rewrite, 2010-03-12), we introduced a test case that verifies that the > config setting `notes.rewriteRef` can be overridden via the environment > variable `GIT_NOTES_REWRITE_REF`. > > Back when it was introduced, it relied on a side effect of an earlier > test case that configured `core.noteRef` to point to `refs/notes/other`. > > In 908a320363 (t3301: modernize style, 2014-11-12), this side effect was > removed. > > The test case *still* passed, but for the wrong reason: we no longer > overrode the rewrite ref, but there simply was nothing to rewrite > anymore, as the overridden notes ref was "modernized" away.
Wow. Thanks for digging thru this
Show 11 quoted lines
> Let's let that test case pass for the correct reason again. > > To make sure of that, let's change the idea of the original test case: > it configured `notes.rewriteRef` to point to the actual notes ref, > forced that to be ignored and then verified that the notes were *not* > rewritten. > > By turning that idea upside down (configure the `notes.rewriteRef` to > another notes ref, override it via the environment variable to force the > notes to be copied, and then verify that the notes *were* rewritten), we > make it much harder for that test case to pass for the wrong reason.
Yup. I agree that testing positive side, expressing what we want to happen in a more explicit manner, is always a better alternative.
Will queue. Thanks.
Show 16 quoted lines
> index 84bbf88cf9..704bbc6541 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1120,9 +1120,10 @@ test_expect_success 'GIT_NOTES_REWRITE_REF overrides config' ' > test_config notes.rewriteMode overwrite && > test_config notes.rewriteRef refs/notes/other && > echo $(git rev-parse HEAD^) $(git rev-parse HEAD) | > - GIT_NOTES_REWRITE_REF= git notes copy --for-rewrite=foo && > + GIT_NOTES_REWRITE_REF=refs/notes/commits \ > + git notes copy --for-rewrite=foo && > git log -1 >actual && > - test_cmp expect actual > + grep "replacement note 3" actual > ' > > test_expect_success 'git notes copy diagnoses too many or too few parameters' '