Re: [GSoC PATCH 2/2] t4074: add test for diffstat width when prefix contains ANSI chars
LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:
> Add test checking the calculation of the diffstat display width when
> the line_prefix contains ANSI characters.
Hmph, who stuffs the "line_prefix" and with what?
Show 6 quoted lines
> Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
> ---
> t/meson.build | 1 +
> t/t4074-diff-stat-width-with-line-prefix.sh | 42 +++++++++++++++++++++
> 2 files changed, 43 insertions(+)
> create mode 100755 t/t4074-diff-stat-width-with-line-prefix.sh
Do we need a brand new test script only to host just two tests? Is it too cumbersome to modify existing test scripts that already test "git log --graph"?
Show 5 quoted lines
> +setup () {
> + rm -rf * ".git" &&
> + git init &&
> + git config color.diff always
> +}> +test_expect_success 'check width with max name-width' '
> + setup &&
Why? Shouldn't something like
test_config color.diff always &&
be sufficient?
> + touch "${FILE_MAX}" &&Use of "touch" when you do not care about the timestamp of the resulting file is misleading. If you only care about its existence, do something like
>"${FILE_MAX}" &&instead.
Show 5 quoted lines
> + git add . &&
> + git commit -m "init" &&
> + echo "text" >"${FILE_MAX}" &&
> + git add . &&
> + git commit -m "text" &&OK, so we have two commits, one adds an empty file, followed by another that adds one line of "text" to the file.
> + git log --graph --stat >out &&
> + grep "${FILE} | 1" out
> +'Use "test_grep" instead of "grep" to make it easier to debug when things break, perhaps?
It is unclear what we are exactly looking for. What's the failure mode and how the miscounting of the width of the line-prefix contribute to that failure?
We have two commits, "log" without "--reverse" would give the creation of an empty file i.e., "file | 0" first and then one line addition to the file i.e., "file | 1 +" next. We only care about "${FILE} | 1" existing in the output and we do not bother checking what comes at the beginning of the line before "${FILE}" or what comes on the line fter "| 1", because we can detect the breakage we expect to see only by looking at the middle of the line? How would that work? The test deserves a bit more comment to help readers who wonder about these things.
Side note. I think I know the answer to these questions,
but this project is not about me ;-) but other future
contributors also would need help when they encounter this
test and want to learn what it is testing.
Show 13 quoted lines
> +test_expect_success 'check width with longer name-width' '
> + setup &&
> + touch "${FILE_MAX}+" &&
> + git add . &&
> + git commit -m "init" &&
> + echo "text" >"${FILE_MAX}+" &&
> + git add . &&
> + git commit -m "text" &&
> + git log --graph --stat >out &&
> + grep "${FILE_LONGER} | 1" out
> +'
> +
> +test_doneInstead of doing the repository construction twice, you can see the same effect by running "git log --graph --stat" on the same history with different values exported on COLUMNS environment variable, which I think would be easier to see and simpler to debug.
Something along the lines of ...
COLUMNS=80 git log --graph --stat >out &&
test_decode_color out >out.decoded &&
test_grep "^<RED>|<RESET> ${FILE_MAX} | 1 <GREEN>+<RESET>$" out.decoded && COLUMNS=79 git log --graph --stat >out &&
test_decode_color out >out.decoded &&
test_grep "^<RED>|<RESET> ${FILE_TRUNCATED} | 1 <GREEN>+<RESET>$" out.decoded... perhaps?
> +# The terminal, during a test, should default to a width of 80 columns
> +FILE_MAX="filename-with-exact-length-to-take-the-max-amount-of-space-in-diffstat"
> +FILE_LONGER="...name-with-exact-length-to-take-the-max-amount-of-space-in-diffstat+"
LorenzoPegorari