| Message ID | 20250408114841.58592-2-anthonywang03@icloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t9811: Improve test coverage and clarity | expand |
Anthony Wang <anthonywang513@gmail.com> writes: > Subject: Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity "Improve" -> "improve". I am not sure about "clarity", but I agree that the main thrust of this change is no longer "avoid hiding exit status of git behind pipe", but more about "use 'show-ref --verify' to validate the right thing (meaning: grepping just for a string that could be a substring is an unreliable test)". Perhaps that is giving more clarity? coverage to test negative outcome is "while at it", just like we (incidentally) lost pipes that used to hide exit status, so I am not sure if it deserves mention on the commit title. t9811: be more precise to check tag creation The tests grep tagnames they expect to exist from "git tag" output, which can be fooled by false positive if an unexpected tag whose name has the expected tagname as its substring. Fix them by using "git show-ref --verify" instead. While we are at it, add a negative test to verify that a tag that is involved in earlier tests that is not supposed to appear in the result does indeed not appear in the resulting repository. Incidentally, this would also correct the problem the original had, which lost the exit status of "git tag" that was placed upstream of a pipe. or something, perhaps? Also you'd need to sign-off your patch. Thanks. > Remove the pipe following the `git tag`, ensuring the exit code is not > hidden. Add explicit verification to check for expected and unexpected > tags, increasing specificity and future-proofing a portion of the test. > > --- > t/t9811-git-p4-label-import.sh | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh > index 5ac5383fb7..593de09eb4 100755 > --- a/t/t9811-git-p4-label-import.sh > +++ b/t/t9811-git-p4-label-import.sh > @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' ' > cd "$git" && > git p4 sync --import-labels && > > - git tag | grep TAG_F1 && > - git tag | grep -q TAG_F1_1 && > - git tag | grep -q TAG_F1_2 && > + git tag && > + git show-ref --verify refs/tags/TAG_F1_1 && > + git show-ref --verify refs/tags/TAG_F1_2 && > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY && > > cd main && > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' ' > git p4 rebase --verbose && > git p4 submit --verbose && > git tag && > - git tag | grep TAG_F1_1 > + git show-ref --verify refs/tags/TAG_F1_1 && > ) && > ( > cd "$cli" &&
On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote: > t9811: be more precise to check tag creation > > The tests grep tagnames they expect to exist from "git tag" s/tagnames/tag names/ perhaps? > output, which can be fooled by false positive if an unexpected > tag whose name has the expected tagname as its substring. Fix > them by using "git show-ref --verify" instead. > > While we are at it, add a negative test to verify that a tag > that is involved in earlier tests that is not supposed to appear > in the result does indeed not appear in the resulting > repository. > > Incidentally, this would also correct the problem the original > had, which lost the exit status of "git tag" that was placed > upstream of a pipe. > > or something, perhaps? Yes, better and much more illuminating. > > - git tag | grep TAG_F1 && > > - git tag | grep -q TAG_F1_1 && > > - git tag | grep -q TAG_F1_2 && > > + git tag && > > + git show-ref --verify refs/tags/TAG_F1_1 && > > + git show-ref --verify refs/tags/TAG_F1_2 && > > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY && Do we still need the standalone `git tag` invocation above? > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' ' > > git p4 submit --verbose && > > git tag && > > - git tag | grep TAG_F1_1 > > + git show-ref --verify refs/tags/TAG_F1_1 && Similarly, it's not clear why there is a standalone `git tag` invocation here. Does it buy us anything or am I missing something obvious? The originating commit[*] doesn't explain its purpose. [*] e71f6a53e2 (git p4: add test for tag import/export enabled via config, 2012-05-11)
On Tue, Apr 8, 2025 at 11:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Tue, Apr 8, 2025 at 5:21 PM Junio C Hamano <gitster@pobox.com> wrote: > > t9811: be more precise to check tag creation > > > > The tests grep tagnames they expect to exist from "git tag" > > s/tagnames/tag names/ perhaps? How does "t9811: be more precise to check importing of tags" sound? The tags are created in the p4 repository, and what we care about is if `git p4 sync --import-labels` correctly imports the labels from the p4 repository. Specifying tags instead of tag indicates that there are multiple tags, which is the whole purpose of the test. > > > output, which can be fooled by false positive if an unexpected > > tag whose name has the expected tagname as its substring. Fix > > them by using "git show-ref --verify" instead. > > > > While we are at it, add a negative test to verify that a tag > > that is involved in earlier tests that is not supposed to appear > > in the result does indeed not appear in the resulting > > repository. > > > > Incidentally, this would also correct the problem the original > > had, which lost the exit status of "git tag" that was placed > > upstream of a pipe. > > > > or something, perhaps? > > Yes, better and much more illuminating. I see, that message does communicate the intentions of the changes much better than I did. I will expand the message and write something along the lines of what you have recommended. > > > > - git tag | grep TAG_F1 && > > > - git tag | grep -q TAG_F1_1 && > > > - git tag | grep -q TAG_F1_2 && > > > + git tag && > > > + git show-ref --verify refs/tags/TAG_F1_1 && > > > + git show-ref --verify refs/tags/TAG_F1_2 && > > > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY && > > Do we still need the standalone `git tag` invocation above? The original intent of the patch was to expose the exit code of `git tag`. By keeping the standalone `git tag` we are able to pick up the exit code if there are any issues, such as if `git p4 sync --import-labels` somehow breaks `git tag`. I believe this is the intent of the standalone `git tag` in the section below, and as such, I added it to the top section in order to test the `git tag` functionality, as we removed the other `git tag` instances. However, I can understand if it is unnecessary, and I will remove it. > > > > @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' ' > > > git p4 submit --verbose && > > > git tag && > > > - git tag | grep TAG_F1_1 > > > + git show-ref --verify refs/tags/TAG_F1_1 && > > Similarly, it's not clear why there is a standalone `git tag` > invocation here. Does it buy us anything or am I missing something > obvious? The originating commit[*] doesn't explain its purpose. > Again, as we have removed the other `git tag` instances, it might still have value to test the result of running the `git tag` command. > [*] e71f6a53e2 (git p4: add test for tag import/export enabled via > config, 2012-05-11) Thank you very much for the valuable feedback, it has been very eye opening to see how you two approach patching code like this. I wanted to bump one of my previous questions, as I am very curious about what the best practice is here: To this point, I have a question about when to modify code when making patches. My understanding is that we should try to only modify the code necessary to fix the bug, and not modify other parts of the code. However, because in this case the test itself does not correctly test for the intended behavior, we should modify because we are already touching this piece of code. Is this correct? Would it then be desired to check the rest of the tests in this file for further oversights and correct them as well, or would that be overstepping boundaries?
Anthony Wang <anthonywang513@gmail.com> writes: >> > The tests grep tagnames they expect to exist from "git tag" >> >> s/tagnames/tag names/ perhaps? > > How does "t9811: be more precise to check importing of tags" sound? Excellent. >> > > + git tag && >> > > + git show-ref --verify refs/tags/TAG_F1_1 && >> > > + git show-ref --verify refs/tags/TAG_F1_2 && >> > > + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY && >> >> Do we still need the standalone `git tag` invocation above? > > The original intent of the patch was to expose the exit code of > `git tag`. Is it? I somehow thought that "git tag" is not what is being tested by this script. Rather, it assumed that "git tag" works perfectly well, and validated what "git p4" left in the resulting repository based on that assumption, i.e. "git tag" works perfectly well to tell us what tags are in the repository. It is true that "git tag" to list all available tags may fail, but then catching that is outside the scope of this script no? It is even more so, since now we do not even depend on the correct operation of "git tag" anymore to validate what "git p4" did---we now use "show-ref --verify" for that, so we do not even care if "git tag" segfaults in this part of the test, no? > However, because in this case the test itself does not correctly test > for the intended behavior, we should modify because we are already > touching this piece of code. Is this correct? Would it then be desired > to check the rest of the tests in this file for further oversights and > correct them as well, or would that be overstepping boundaries? Just like any real world problems, there unfortunately is no bright red line between "yeah these are related enough and in the same spot and it is better to clean up while we are at it" and "that is way too much for this single topic" that can be described in a textbook. A rule of thumb I personally use is to put me in the shoes of an imaginary typical Git developer with moderate competence, who hasn't seen or worked on a particular part of the system being updated. If I can easily imagine that the developer can clearly see a need for clean up (in this case, "the part of the code only tests positive results and forgets about negative check") while fixing something else (in this case, "use of 'git tag' piped to 'grep' has at least two problems, loss of exit code and false match") and the additional effort would be smaller than 10-20 minutes, I'd say it would be worth doing and anything larger would be better to leave to another day, but a lot of ingredients in that statement are very much subjective (starting from "what's the average competence level we expect from our people?").
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh index 5ac5383fb7..593de09eb4 100755 --- a/t/t9811-git-p4-label-import.sh +++ b/t/t9811-git-p4-label-import.sh @@ -95,9 +95,10 @@ test_expect_success 'two labels on the same changelist' ' cd "$git" && git p4 sync --import-labels && - git tag | grep TAG_F1 && - git tag | grep -q TAG_F1_1 && - git tag | grep -q TAG_F1_2 && + git tag && + git show-ref --verify refs/tags/TAG_F1_1 && + git show-ref --verify refs/tags/TAG_F1_2 && + test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY && cd main && @@ -208,7 +209,7 @@ test_expect_success 'use git config to enable import/export of tags' ' git p4 rebase --verbose && git p4 submit --verbose && git tag && - git tag | grep TAG_F1_1 + git show-ref --verify refs/tags/TAG_F1_1 && ) && ( cd "$cli" &&