| Message ID | 20200829225648.11971-1-alipman88@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | e6d5a11fed0bd6871a16e9004d960f1447529e67 |
| Headers | show |
| Series | t3200: clean side effect of git checkout --orphan | expand |
On Sat, Aug 29, 2020 at 6:57 PM Aaron Lipman <alipman88@gmail.com> wrote: > The "refuse --edit-description on unborn branch for now" test in t3200 > switches to an orphan branch, causing subsequent git commands > referencing HEAD to fail. Avoid this side-effect by switching back to > master after that test finishes. > > This has gone undetected, as the next effected test expects failure - > but it currently fails for the wrong reason. s/effected/affected In fact, the three tests following the orphan test all expect failure (though I didn't check if they also fail for the wrong reason), and the following test which doesn't expect failure has an explicit "git checkout master" early on, which explains why it was not impacted by this problem. > Verbose output of the next test referencing HEAD, > "--merged is incompatible with --no-merged": > > fatal: malformed object name HEAD > > Which this commit corrects to: > > error: option `no-merged' is incompatible with --merged > > Signed-off-by: Aaron Lipman <alipman88@gmail.com> Description and actual fix make perfect sense.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Sat, Aug 29, 2020 at 6:57 PM Aaron Lipman <alipman88@gmail.com> wrote: >> The "refuse --edit-description on unborn branch for now" test in t3200 >> switches to an orphan branch, causing subsequent git commands >> referencing HEAD to fail. Avoid this side-effect by switching back to >> master after that test finishes. >> >> This has gone undetected, as the next effected test expects failure - >> but it currently fails for the wrong reason. > > s/effected/affected > > In fact, the three tests following the orphan test all expect failure > (though I didn't check if they also fail for the wrong reason), and > the following test which doesn't expect failure has an explicit "git > checkout master" early on, which explains why it was not impacted by > this problem. > >> Verbose output of the next test referencing HEAD, >> "--merged is incompatible with --no-merged": >> >> fatal: malformed object name HEAD >> >> Which this commit corrects to: >> >> error: option `no-merged' is incompatible with --merged >> >> Signed-off-by: Aaron Lipman <alipman88@gmail.com> > > Description and actual fix make perfect sense. Yeah, looks good. Of course, the affected test can be made more defensive to protect the precondition it relies on from getting broken by other tests (i.e. if it refers to HEAD, it should make sure it is on an actual commit). Each test cleaning up after itself is a good discipline to have, but what is "clean" is quite subjective and depends on each test piece in the script X-<. Thanks, will queue.
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 4c0734157b..028c88d1b2 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1287,6 +1287,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' ' ' test_expect_success 'refuse --edit-description on unborn branch for now' ' + test_when_finished "git checkout master" && write_script editor <<-\EOF && echo "New contents" >"$1" EOF
The "refuse --edit-description on unborn branch for now" test in t3200 switches to an orphan branch, causing subsequent git commands referencing HEAD to fail. Avoid this side-effect by switching back to master after that test finishes. This has gone undetected, as the next effected test expects failure - but it currently fails for the wrong reason. Verbose output of the next test referencing HEAD, "--merged is incompatible with --no-merged": fatal: malformed object name HEAD Which this commit corrects to: error: option `no-merged' is incompatible with --merged Signed-off-by: Aaron Lipman <alipman88@gmail.com> --- We might considered updating "--merged is incompatible with --no-merged" tests to not only test for failure, but check the contents of the error message to ensure they fail for the correct reason, e.g.: test_expect_success '--merged is incompatible with --no-merged' ' test_must_fail git branch --merged HEAD --no-merged HEAD 2>error && test_i18ngrep "is incompatible with --merged" error ' However, I intend to submit a patch enabling ref-filter to accept multiple merged/no-merged filters, which would make said updates irrelevant. t/t3200-branch.sh | 1 + 1 file changed, 1 insertion(+)