Re: [PATCH] t6016: move to lib-log-graph.sh framework
- From
Antonio Russo <aerusso@aerusso.net>
- Date
- Jan 6, 2021, 15:21 UTC
- Message-ID
- <d01e2830-d579-67c6-1076-ed44cff27ad7@aerusso.net>
- In-Reply-To
- <xmqq1reywt7x.fsf@gitster.c.googlers.com>
On 1/5/21 10:05 PM, Junio C Hamano wrote:
Show 31 quoted lines
> Abhishek Kumar <abhishekkumar8222@gmail.com> writes: > >> On Sun, Jan 03, 2021 at 07:30:35PM -0700, Antonio Russo wrote: >>> t6016 manually reconstructs git log --graph output by using the reported >>> commit hashes from `git rev-parse`. Each tag is converted into an >>> environment variable manually, and then `echo`-ed to an expected output >>> file, which is in turn compared to the actual output. >>> >>> The expected output is difficult to read and write, because, e.g., >>> each line of output must be prefaced with echo, quoted, and properly >>> escaped. Additionally, the test is sensitive to trailing whitespace, >>> which may potentially be removed from graph log output in the future. >>> >>> In order to reduce duplication, ease troubleshooting of failed tests by >>> improving readability, and ease the addition of more tests to this file, >>> port the operations to `lib-log-graph.sh`, which is already used in >>> several other tests, e.g., t4215. Give all merges a simple commit >>> message, and use a common `check_graph` macro taking a heredoc of the >>> expected output which does not required extensive escaping. >>> >> >> Glad to see others using `lib-log-graph.sh` as well! >> >> The changes look alright to me but maybe you could have split the two >> changes into two different commits: Using tags directly instead of >> environment variables and using `check_graph` instead of manually >> `echo`-ing to an expected output and comparing with the actual output. > > Perhaps. Also I am wondering if the tests still need to create tags > after this change---isn't the output all matched by the commit title > now?
You are correct that the A4 and A6 tags can be removed without affecting the output. In fact, A4 is basically immediately deleted (in the second test). I can remove that, if we want to stop testing the tag deletion logic here. I suppose that is sufficiently validated elsewhere in the test suite.
There's a (weak IMO) argument to keep the A6 tag, since it might in principle affect the output at later stages, since it is not deleted and calls are made with --simplify-by-decoration. Of course, it isn't needed in order to be displayed, so we're only testing that that the merge commit A6 shows up when it *has* a tag, and --simplify-by-decoration is used. That failure mode certainly seems perverse!
My guiding principle when I made this patch was to be as minimally invasive as possible, while allowing modifications to this file to be pleasant---which I must admit is my ulterior motive.
I can certainly remove these "extraneous" tags if desired.
Best, Antonio
Show 55 quoted lines
> That is ...
>
>>> . ./test-lib.sh
>>> +. "$TEST_DIRECTORY"/lib-log-graph.sh
>>> +
>>> +check_graph () {
>>> + cat >expect &&
>>> + lib_test_cmp_graph --format=%s "$@"
>>> +}
>
> ... this only shows the title, and ...
>
>>> - git merge B C &&
>>> + git merge B C -m A4 &&
>
> ... that is why we need to have the title here.
>
>>> git tag A4 &&
>
> Now, does this A4 used anywhere?
>
>>> - # Store commit names in variables for later use
>>> - A1=$(git rev-parse --verify A1) &&
>>> - A2=$(git rev-parse --verify A2) &&
>>> - A3=$(git rev-parse --verify A3) &&
>>> - A4=$(git rev-parse --verify A4) &&
>
> It certainly used to be needed here, but ...
>>> + check_graph --all <<-\EOF
>>> + * A7
>>> + * A6
>>> + |\
>>> + | * C4
>>> + | * C3
>>> + * | A5
>>> + | |
>>> + | \
>>> + *-. | A4
>
> ... not anymore in the new version.
>
>>> + |\ \|
>>> + | | * C2
>>> + | | * C1
>>> + | * | B2
>>> + | * | B1
>>> + * | | A3
>>> + | |/
>>> + |/|
>>> + * | A2
>>> + |/
>>> + * A1
>>> + EOF
>>> +'
>>>