UnkindPartition / tasty

Modern and extensible testing framework for Haskell
640 stars 110 forks source link

Mention the proper env vars for usage in MinGW #404

Closed jasagredo closed 11 months ago

jasagredo commented 11 months ago

If one doesn't set that flag, by default any argument /.../ gets converted to a path by MSYS2 (see).

❯ cabal run my-testsuite -- -p "/my pattern/"
option -p: Could not parse: C:/msys64/my pattern/ is not a valid pattern
...
❯ MSYS2_ARG_CONV_EXCL=* cabal run my-testsuite -- -p "/my pattern/"
<it works>

This will print the following advise on failing tests when running on what looks like a MinGW enviroment:

          Use -p '/my pattern/' to rerun this test only.
          If running in a MinGW shell, set the environment variable 'MSYS2_ARG_CONV_EXCL=*' or the pattern above will be mangled.

It still uses the "If" formulation because PowerShell running the mingw haskell toolchain won't need this parameter (modulo haskell/cabal#9519):

PS C:\Users\Javier\example> cabal run my-testsuite -- -p "/my pattern/"
<works>
Bodigrim commented 11 months ago

FAQ recommends setting MSYS_NO_PATHCONV=1. Is MSYS2_ARG_CONV_EXCL=* better or different?

It would be better to add the instruction to the error message for failed parse instead of repeating it after each failed test.

jasagredo commented 11 months ago

I was unable to make it work with MSYS_NO_PATHCONV but I can try again.

And I agree it would be better to do it in the parse error message. Good point, will do.

jasagredo commented 11 months ago
❯ MSYS_NO_PATHCONV=1 cabal run cardano-test -- -p "/ExtLedgerState_Conway/"
option -p: Could not parse: C:/msys64/ExtLedgerState_Conway/ is not a valid pattern
...

❯ MSYS2_NO_PATHCONV=1 cabal run cardano-test -- -p "/ExtLedgerState_Conway/"
option -p: Could not parse: C:/msys64/ExtLedgerState_Conway/ is not a valid pattern
...
jasagredo commented 11 months ago

As the error in parsing is created by mkOptionCLParser using just the name of the parser, in order to not print this error for all parsers (which wouldn't make sense), would it be better to add a field to IsOption v something like:

class Typeable v => IsOption v where
  -- | Optional additional hints to present on the CLI when this option 
  -- fails to parse, for example OS specific hints.
  additionalHints :: Tagged v (Maybe String)

?

Bodigrim commented 11 months ago

Maybe we can unmangle automatically what MSYS has mangled? That would be the best solution.

jasagredo commented 11 months ago

I don't think that would be a good approach, I think tasty should try to interpret the input as a pattern always, and fail if it is not a pattern.

At most the slashes could be optional maybe? That would avoid MSYS2 mangling the parameter.

(The above is my current opinion, I don't know the internals of tasty or the overall design and goals so I could be wrong)

Bodigrim commented 11 months ago

MSYS_NO_PATHCONV used to work for me in MinGW shells like Git Bash. It’s surprising that it does not work for you, has something changed on MSYS side?..

There are two low-hanging fruits: we can update README.md with more robust instructions and we can also extend --help output wrt --pattern. Other than that I’d prefer to extend the parser of patterns to demangle things: after all a user might not be in position to manipulate environment variables at all.

jasagredo commented 11 months ago

I found why the env var does not work. It is specific to the MSYS2 fork of Git for Windows https://github.com/git-for-windows/msys2-runtime/commit/f874ac108a and doesn't apply when one installs MinGW directly instead of via Git for Windows.

jasagredo commented 11 months ago

Let's just update the README and let it be.

jasagredo commented 11 months ago

Let me know if this looks good now.

Bodigrim commented 11 months ago

Thanks!