Add option to set max line length

brandon-leapyear commented 1 year ago

Currently, the test output puts the result statuses all at the same column, which matches the longest test name. This causes the tests to wrap when you have just one really long test name:

test 1:
            OK
test 2:
            OK
test 5:
            OK
test 10:
            OK
test 100:
            OK
(fromMySpecialType . toMySpecialType) is idempotent except when someOption
Name=False: OK
some other longish named test:
            OK

It'd be nice to set a max line length, whereby the "find longest line" calculation only accounts for lines shorter than the given configuration, and longer lines will just wrap as usual. e.g. with the above output and --max-line-length=10:

test 1:   OK
test 2:   OK
test 5:   OK
test 10:  OK
test 100: OK
(fromMySpecialType . toMySpecialType) is idempotent except when someOption
Name=False: OK
some other longish named test: OK

Bodigrim commented 1 year ago

There are plenty of things one might wish to configure in consoleTestReporter, but it would be overwhelming for casual users to be presented with a screen-long list of display options which they do not care about.

I suggest we rather export more helpers from Test.Tasty.Ingredients.ConsoleReporter, so that other developers could easily implement their own reporter with required capabilities and options.

brandon-leapyear commented 1 year ago

:sparkles: This is an old work account. Please reference @brandonchinn178 for all future communication :sparkles:

I'm sympathetic to that. Alternatively, what do you think about doing something like GHC's --help vs --show-all-options? Just fantasizing here, it'd be cool if we could do something like what pytest does:

$ stack test --ta --help-full

Mmm... tasty test suite

Usage: my-test-suite [OPTIONS] PATTERN

General options:
  -h,--help                Show this help text
  -t,--timeout DURATION    Timeout for individual tests (suffixes: ms,s,m,h;
                           default: s)
  -j,--num-threads NUMBER  Number of threads to use for tests execution
                           (default: # of cores/capabilities)
  ...

Options for 'ListTestReporter':
  -l,--list-tests          Do not run the tests; just print their names

Options for 'tasty-quickcheck':
  --quickcheck-replay SEED Random seed to use for replaying a previous test run
                           (use same --quickcheck-max-size)
  ...

(As a side note, it'd be cool to have PATTERNs just be plain positional arguments)

Then the --help flag would just be the --help-full flag, except only showing "important" options and a message saying "See all options with --help-full". Overall, this would require significant architecture changes (e.g. specifying a "group" for OptionDescription), so probably not worth it, but a guy can dream 💭

Bodigrim commented 1 year ago

Breaking changes to existing providers/ingredients are not on the table.

bfrk commented 1 year ago

@Bodigrim I agree that adding lots of options for customizing the output format is probably not a good idea for the reasons you stated. But this is only a problem if the option is visible to the user. What I had in mind was a way to customize the format (only) programatically.

A generic way to solve the tension between the two goals (customizability vs. keeping the user interface simple) would be to split the IsOption class into layers: a base class IsOption that has only optionName and defaultValue as members, and on top of that a class IsInteractiveOption with the rest of the methods (those which obtain to parsing and showing of options). The code that uses (queries, manipulates) options only needs the basic IsOption layer; only the implementation of the command line parser, including help generation etc. needs the methods from IsInteractiveOption.

Bodigrim commented 1 year ago

Can we measure the width of the terminal and switch automatically?

bfrk commented 1 year ago

Yes, good idea. There is https://hackage.haskell.org/package/terminal-size

I have made a contribution to that recently ;-) in order to better support non-native Windows terminals. In fact, isJust size is, to the best of my knowledge, currently the most portable and correct way to check whether stdout is a terminal device.

My preferred choice in case we hit the max line length would be to cut off overlong test names with an elipse e.g

      test_with_a_very_very_long_n[...] OK (0.1s)

bfrk commented 1 year ago

Actually, the ultimate solution may be to always use the full terminal width. For users who like a very wide terminal we could connect the test name and the result with dots:

All Tests
  Named RepoPatchV3
    using V2.Prim wrapper for Prim.V1
      merge commute ............................................................................... OK (0.02s)
        +++ OK, passed 100 tests; 27 discarded (76% nontrivial).
      resolutions are invariant under reorderings ................................................. OK (0.03s)
        +++ OK, passed 100 tests.
      Rebase patches
        duplicate in rebase fixup has a conflicted effect ......................................... OK
      Unwind
        unwind named succeeds ..................................................................... OK (0.18s)

Bodigrim commented 1 year ago

I'd rather be as conservative with tasty output as possible; no dots please.

bfrk commented 1 year ago

I am not sure I understand what you mean with "conservative". If you mean "keep it as is" then there is no point to this discussion, so I assume you rather mean "avoid fancy markup". I might actually agree with that POV, though that implies we should remove tabular output altogether (as in my original proposal minus the option). But if we take the tabular output as given, I think the dots make it much easier to read (i.e. line-up the test name with the result).

I won't insist on it, though. My main point was (1) to always use the full terminal width and (2) to cut off overlong test names (with elipses or without). Indeed, one of the reasons I think this is superior is that it avoids (as far as possible) placing the result under group names (which looks "wrong" to me and which I therefore find distracting). Here is an another real life example demonstrating what I mean:

  tests/network/issue2090-transfer-mode.sh
    Darcs1,Patience,WithIndex,WithCache: OK (2.15s)
    Darcs2,Patience,WithIndex,WithCache: OK (2.18s)
    Darcs3,Patience,WithIndex,WithCache: OK (2.23s)
  tests/network/issue2545_command-execution-via-ssh-uri-local.sh
    Darcs1,Patience,WithIndex,WithCache: OK (0.50s)
    Darcs2,Patience,WithIndex,WithCache: OK (0.52s)
    Darcs3,Patience,WithIndex,WithCache: OK (0.48s)
  tests/network/issue2545_command-execution-via-ssh-uri.sh
    Darcs1,Patience,WithIndex,WithCache: OK (1.13s)
    Darcs2,Patience,WithIndex,WithCache: OK (1.30s)
    Darcs3,Patience,WithIndex,WithCache: OK (1.33s)
  tests/network/issue2608-clone-http-packed-outdated.sh
    Darcs1,Patience,WithIndex,WithCache: OK (0.45s)
    Darcs2,Patience,WithIndex,WithCache: OK (0.36s)
    Darcs3,Patience,WithIndex,WithCache: OK (0.35s)

vs.

  tests/network/issue2090-transfer-mode.sh
    Darcs1,Patience,WithIndex,WithCache:                                OK (2.15s)
    Darcs2,Patience,WithIndex,WithCache:                                OK (2.18s)
    Darcs3,Patience,WithIndex,WithCache:                                OK (2.23s)
  tests/network/issue2545_command-execution-via-ssh-uri-local.sh
    Darcs1,Patience,WithIndex,WithCache:                                OK (0.50s)
    Darcs2,Patience,WithIndex,WithCache:                                OK (0.52s)
    Darcs3,Patience,WithIndex,WithCache:                                OK (0.48s)
  tests/network/issue2545_command-execution-via-ssh-uri.sh
    Darcs1,Patience,WithIndex,WithCache:                                OK (1.13s)
    Darcs2,Patience,WithIndex,WithCache:                                OK (1.30s)
    Darcs3,Patience,WithIndex,WithCache:                                OK (1.33s)
  tests/network/issue2608-clone-http-packed-outdated.sh
    Darcs1,Patience,WithIndex,WithCache:                                OK (0.45s)
    Darcs2,Patience,WithIndex,WithCache:                                OK (0.36s)
    Darcs3,Patience,WithIndex,WithCache:                                OK (0.35s)

Bodigrim commented 11 months ago

I'd take a PR implementing the following algorithm:

Measure terminal width termW :: Maybe Int if in terminal, otherwise set it to Nothing.
Compute resultColumn = min(longest indented test name, termW - k), wherek accounts for a typical length of FAIL (3.48 s). I'd say k = 20 is a good option, but one have to check various test providers, e. g., tasty-expected-failure tends to be longish.
Pad shorter test names up to resultColumn, do not pad longer test names at all.

Tricky part: check that this interacts with progress reporting well.

As for measuring terminal width, I'd probably stick to getTerminalSize from ansi-terminal for now to keep dependency footprint as is, including Windows. (Full disclosure: I'm reluctant for tasty to depend on bytestring)

brandonchinn178 commented 11 months ago

This change isn't a breaking change right? So it's not urgent for the 1.5 release?

Bodigrim commented 11 months ago

I can imagine making this change in a minor release, yes. But that's assuming that the plan outlined above works as expected, which I'm far from certain. ConsoleReporter is a complicated piece, and I'd rather make all behavioral changes in one go before cutting an RC, so that we can gather some feedback before releasing.

If there is a genuine interest to fix the issue, but no resources at the moment, I'd rather postpone the release of tasty-1.5 for later.

UnkindPartition / tasty

Add option to set max line length #342