diff mbox series

progress.c tests: fix breakage with COLUMNS != 80

Message ID patch-1.1-cba5d88ca35-20210621T070114Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series progress.c tests: fix breakage with COLUMNS != 80 | expand

Commit Message

Ævar Arnfjörð Bjarmason June 21, 2021, 7:01 a.m. UTC
The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
broke under anything except COLUMNS=80, i.e. when running them under
the "-v" mode under a differently sized terminal.

Let's set the expected number of COLUMNS at the start of the test to
fix that bug. It's handy not do do this in test-progress.c itself, in
case we'd like to test for a different number of COLUMNS, either
manually or in a future test.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0500-progress-display.sh | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jeff King June 23, 2021, 11:48 p.m. UTC | #1
On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:

> The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
> broke under anything except COLUMNS=80, i.e. when running them under
> the "-v" mode under a differently sized terminal.
> 
> Let's set the expected number of COLUMNS at the start of the test to
> fix that bug. It's handy not do do this in test-progress.c itself, in
> case we'd like to test for a different number of COLUMNS, either
> manually or in a future test.

Hmm. So I can easily reproduce the problem here, and your patch fixes
it. But my first thought was: shouldn't test-lib.sh be handling this to
give all of the scripts a uniform environment?

And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
a bad setting of $COLUMNS, but rather that in "-v" mode, the
sub-command's stderr is hooked to our tty, and term_columns() is smart
enough to use TIOCGWINSZ to get the value (at least on some platforms).

Setting $COLUMNS again in the environment fixes it, because we prefer
that value to trying the ioctl.

So I don't think what you have here is wrong (though the commit message
is a little misleading). But it seems like the original intent of our
"unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
a known value there (like 80), rather than unsetting it.

I admit this a _bit_ of a nitpick (since as far as we know none of the
other scripts care about the terminal width), so I'm OK with this as-is
if you feel strongly the other way.

-Peff
SZEDER Gábor June 24, 2021, 5:12 a.m. UTC | #2
On Wed, Jun 23, 2021 at 07:48:25PM -0400, Jeff King wrote:
> On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
> > broke under anything except COLUMNS=80, i.e. when running them under
> > the "-v" mode under a differently sized terminal.
> > 
> > Let's set the expected number of COLUMNS at the start of the test to
> > fix that bug. It's handy not do do this in test-progress.c itself, in
> > case we'd like to test for a different number of COLUMNS, either
> > manually or in a future test.
> 
> Hmm. So I can easily reproduce the problem here, and your patch fixes
> it. But my first thought was: shouldn't test-lib.sh be handling this to
> give all of the scripts a uniform environment?
> 
> And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
> a bad setting of $COLUMNS, but rather that in "-v" mode, the
> sub-command's stderr is hooked to our tty, and term_columns() is smart
> enough to use TIOCGWINSZ to get the value (at least on some platforms).
> 
> Setting $COLUMNS again in the environment fixes it, because we prefer
> that value to trying the ioctl.
> 
> So I don't think what you have here is wrong (though the commit message
> is a little misleading).

It is misleading indeed and needs to be updated.  I did my own
analysis and arrived to the same conclusions wrt COLUMNS being unset
vs. the ioctl() and stderr being a tty.

> But it seems like the original intent of our
> "unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
> a known value there (like 80), rather than unsetting it.
> 
> I admit this a _bit_ of a nitpick (since as far as we know none of the
> other scripts care about the terminal width), so I'm OK with this as-is
> if you feel strongly the other way.

I remember one commit-graph test that does check the number of lines
in the progress output, assuming one progress line per commit-graph
layer, which breaks when we break the progress line in a too narrow
terminal.  Running './t5324-split-commit-graph.sh -v -i' in a 46
column wide terminal fails for me, but succeeds with 47 columns.

I do suggest setting COLUMNS=80 in 'test-lib.sh'.
Jeff King June 24, 2021, 5:40 a.m. UTC | #3
On Thu, Jun 24, 2021 at 07:12:53AM +0200, SZEDER Gábor wrote:

> > I admit this a _bit_ of a nitpick (since as far as we know none of the
> > other scripts care about the terminal width), so I'm OK with this as-is
> > if you feel strongly the other way.
> 
> I remember one commit-graph test that does check the number of lines
> in the progress output, assuming one progress line per commit-graph
> layer, which breaks when we break the progress line in a too narrow
> terminal.  Running './t5324-split-commit-graph.sh -v -i' in a 46
> column wide terminal fails for me, but succeeds with 47 columns.
> 
> I do suggest setting COLUMNS=80 in 'test-lib.sh'.

Thanks for providing a concrete example. I agree we should just set it
for all scripts via test-lib.sh, then.

-Peff
Philip Oakley June 24, 2021, 3:05 p.m. UTC | #4
On 24/06/2021 00:48, Jeff King wrote:
> On Mon, Jun 21, 2021 at 09:01:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> The tests added in 2bb74b53a49 (Test the progress display, 2019-09-16)
>> broke under anything except COLUMNS=80, i.e. when running them under
>> the "-v" mode under a differently sized terminal.
>>
>> Let's set the expected number of COLUMNS at the start of the test to
>> fix that bug. It's handy not do do this in test-progress.c itself, in
>> case we'd like to test for a different number of COLUMNS, either
>> manually or in a future test.
> Hmm. So I can easily reproduce the problem here, and your patch fixes
> it. But my first thought was: shouldn't test-lib.sh be handling this to
> give all of the scripts a uniform environment?
>
> And indeed, we _do_ unset COLUMNS there. So I think the problem isn't
> a bad setting of $COLUMNS, but rather that in "-v" mode, the
> sub-command's stderr is hooked to our tty, and term_columns() is smart
> enough to use TIOCGWINSZ to get the value (at least on some platforms).
>
> Setting $COLUMNS again in the environment fixes it, because we prefer
> that value to trying the ioctl.
>
> So I don't think what you have here is wrong (though the commit message
> is a little misleading). But it seems like the original intent of our
> "unset COLUMNS" in test-lib.sh would best be fulfilled by setting it to
> a known value there (like 80), rather than unsetting it.
>
> I admit this a _bit_ of a nitpick (since as far as we know none of the
> other scripts care about the terminal width), so I'm OK with this as-is
> if you feel strongly the other way.
>
> -Peff
There has been a similar recent issue [1] on Git for Windows regarding
these settings where different terminals had different views  about the
size of the display window which resulted in incorrect out puts.

Philip

[1] https://github.com/git-for-windows/git/issues/3235
diff mbox series

Patch

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 22058b503ac..66c092a0fe3 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -8,6 +8,11 @@  show_cr () {
 	tr '\015' Q | sed -e "s/Q/<CR>\\$LF/g"
 }
 
+test_expect_success 'setup COLUMNS' '
+	COLUMNS=80 &&
+	export COLUMNS
+'
+
 test_expect_success 'simple progress display' '
 	cat >expect <<-\EOF &&
 	Working hard: 1<CR>