Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paths on windows get mangled #16

Closed
Neppord opened this issue Jan 11, 2019 · 13 comments · Fixed by #31
Closed

Paths on windows get mangled #16

Neppord opened this issue Jan 11, 2019 · 13 comments · Fixed by #31
Assignees
Milestone

Comments

@Neppord
Copy link

Neppord commented Jan 11, 2019

I get this error for a very simple test

Call to `git diff` failed: (ExitFailure 1,"","error: Could not access 'C:UsersneppordAppDataLocalTempver6F55.golden'\n")

It seams like all the "\" are gone and that it cant find the correct file.

@phile314
Copy link
Owner

How exactly are you calling tasty-silver? Can you add a minimal, working example which reproduces the problem?

@Neppord
Copy link
Author

Neppord commented Jan 20, 2019

#! /usr/bin/env stack
-- stack --resolver lts-13.4 script


import Test.Tasty.Silver
import Test.Tasty.Silver.Interactive

main = defaultMain $
  goldenVsFile
    "Verify the world"
    "approved.txt"
    "recived.txt"
    (writeFile "recived.txt" "Hello World\n")

The error is triggerd when you have a golden file "approved.txt" that does not contain the approved recived text.

Verify the world: FAIL (0.00s)
                  Result did not match golden value.
                  Diff between actual and golden value:
========================================
Main.hs: Call to `git diff` failed: (ExitFailure 1,"","error: Could not access 'C:UsersneppordAppDataLocalTempVerD8B5.golden'\n")
CallStack (from HasCallStack):
  error, called at .\\Test\\Tasty\\Silver\\Interactive.hs:187:16 in tasty-silver-3.1.12-9wdmuzsaGoxDzww3ZMbbTe:Test.Tasty.Silver.Interactive

@phile314
Copy link
Owner

Looks like the paths are passed to sh, but no escaping takes place:
https://github.com/phile314/tasty-silver/blob/master/Test/Tasty/Silver/Interactive.hs#L184

A quick&dirty fix is probably just escaping backslashes by doubling them. I will see what I can do.

@phile314
Copy link
Owner

I pushed a fix to https://github.com/phile314/tasty-silver/tree/fix_shell_escape . This is untested, as I do not have any Windows systems. Any chance you can test that branch?

@Neppord
Copy link
Author

Neppord commented Feb 6, 2019

What do I do to test the changes?
i'm a stack novice and very new to develop "real" things in Haskell.

@andreasabel
Copy link
Collaborator

I just hit the OP when trying to run the Agda testsuite under cygwin. (After a maybe naive attempt for a Windows CI failed: https://github.com/agda/agda/runs/3450560107?check_suite_focus=true.)

@phile314

This is untested, as I do not have any Windows systems.

A way forward would be multi-platform CI to make sure tasty-silver is portable.

andreasabel added a commit to agda/agda that referenced this issue Aug 28, 2021
The testsuite fails but does not display diffs.
tasty-silver might not yet be portable, see

  phile314/tasty-silver#16
@phile314
Copy link
Owner

I am not using tasty-silver personally much any longer, so I am unlikely to investigate this further. Sorry. I am happy to merge PRs and make new releases if necessary, but don't expect anything beyond that. I am also happy to give maintainer access if anybody is interested.

@andreasabel
Copy link
Collaborator

@phile314 Because Agda depends on tasty-silver, I am happy to become co-maintainer, here and on hackage (https://hackage.haskell.org/package/tasty-silver/maintainers/). I'll likely be conservative (maintaining buildability, CI, fixing critical issues).

@phile314
Copy link
Owner

Sounds good to me, I will add you as co-maintainer here and and on hackage :-)

andreasabel added a commit to agda/agda that referenced this issue Sep 2, 2021
* [ fix #5302 ] make 'cabal test' work for part of the test-suite

'cabal test' will run only the tests defined by 'agda-tests'

* [ #5302 ] CI: work around haskell/cabal#7577

An attempt to get `cabal test` to run after *installing* Agda rather
than just building it.

* [ #5302 ] disable latex-tests for `cabal test`

Mainly for CI, so that we do not have to install LaTeX.

* [ #5302 ] disable compiler-stdlib tests for `cabal test`

Do not work out of the box, need installation of the stdlib (for
`Everything.agda`).

* [ #5302 ] CI: `cabal test` also on macOS and Windows

* [ #5302 ] `cabal test` macOS: work around haskell/cabal#7209

Cabal removes lower-case vowels from package names on macOS, see
haskell/cabal#7209.  Thus Agda might be just Agd in error messages
reporting source locations.

* [ #5302 ] giving up on `cabal test` under Windows

The testsuite fails but does not display diffs.
tasty-silver might not yet be portable, see

  phile314/tasty-silver#16

* [ #5302 ] fix the alert printed by `cabal test`

Correct reference is `v1-install` in `cabal v1-install --run-tests`.

* PR #5536: final polishing
@andreasabel
Copy link
Collaborator

andreasabel commented Sep 2, 2021

A way forward would be multi-platform CI to make sure tasty-silver is portable.

A multi-platform CI is now in place (ee8e237, horray!), but we need to populate the tests directory with system tests that expose the problem on Windows.

@andreasabel
Copy link
Collaborator

I can now reproduce the problem on the Windows CI: https://github.com/phile314/tasty-silver/runs/3527085480?check_suite_focus=true#step:8:12
The diff is not displayed, but the exception isn't propagated up; rather, the test result is simply FAILED (because of the mismatch between golden and actual value).

@andreasabel
Copy link
Collaborator

andreasabel commented Sep 6, 2021

I found out that the indirection via "sh" is to blame, e.g.:

ret <- PTL.readProcessWithExitCode "sh" ["-c", "git diff --no-index --text --exit-code " ++ fGold ++ " " ++ fAct] T.empty

If arguments are not concatenated together as string but passed individually to "git", the mangling does not happen.
In this specific instance, the sh -c wrapper seems superfluous so it is easy to fix. There are other instances where the output is piped into less, this will be less direct to fix:
colorDiff fGold fAct = callProcess "sh" ["-c", "wdiff " ++ fGold ++ " " ++ fAct ++ " | colordiff | less -r > /dev/tty"]

andreasabel added a commit that referenced this issue Sep 8, 2021
This is WIP for debugging purposes, requires the self-tests to be run
in interactive mode.
andreasabel added a commit that referenced this issue Sep 8, 2021
Slashes are accepted as path separator also under Windows, so maybe
this hack is sufficient to fix the problem with name mangling that
occurs with backslashes in filenames passed to "sh".
@andreasabel andreasabel self-assigned this Sep 8, 2021
andreasabel added a commit that referenced this issue Sep 8, 2021
This is WIP for debugging purposes, requires the self-tests to be run
in interactive mode.
andreasabel added a commit that referenced this issue Sep 8, 2021
Slashes are accepted as path separator also under Windows, so maybe
this hack is sufficient to fix the problem with name mangling that
occurs with backslashes in filenames passed to "sh".
andreasabel added a commit that referenced this issue Sep 13, 2021
This is WIP for debugging purposes, requires the self-tests to be run
in interactive mode.
andreasabel added a commit that referenced this issue Sep 13, 2021
Slashes are accepted as path separator also under Windows, so maybe
this hack is sufficient to fix the problem with name mangling that
occurs with backslashes in filenames passed to "sh".
andreasabel added a commit that referenced this issue Sep 13, 2021
…lling

By checking for the existence of external tools like sh, git,
less (also wdiff, colordiff) we are trying to make tasty-silver more
portable.  (E.g. to Windows.)
andreasabel added a commit that referenced this issue Sep 13, 2021
If not for #32, we could leasily enable interaction for some tests
using (localOption $ Interactive true).  Instead, we turn it on in the
CI, breaking generability of haskell-ci.yml.
andreasabel added a commit that referenced this issue Sep 13, 2021
Use the abstractions from System.Process to invoke a shell; should be
more portable.
andreasabel added a commit that referenced this issue Sep 13, 2021
`callCommand "colordiff"` does not work under Cygwin because colordiff
is a python script and needs to be called via a shell that understands
shebangs (which CMD.EXE doesn't do, and the latter is likely behind
`callCommand`).
andreasabel added a commit that referenced this issue Sep 13, 2021
andreasabel added a commit that referenced this issue Sep 18, 2021
Automatically accepting tests in batch mode isn't a good idea, see:
#31 (comment)

So we revert to the previous behavior: interactive tests will fail if
EOI is reached.
andreasabel added a commit that referenced this issue Sep 20, 2021
andreasabel added a commit that referenced this issue Sep 20, 2021
This is WIP for debugging purposes, requires the self-tests to be run
in interactive mode.
andreasabel added a commit that referenced this issue Sep 20, 2021
Slashes are accepted as path separator also under Windows, so maybe
this hack is sufficient to fix the problem with name mangling that
occurs with backslashes in filenames passed to "sh".
andreasabel added a commit that referenced this issue Sep 20, 2021
…lling

By checking for the existence of external tools like sh, git,
less (also wdiff, colordiff) we are trying to make tasty-silver more
portable.  (E.g. to Windows.)
andreasabel added a commit that referenced this issue Sep 20, 2021
If not for #32, we could leasily enable interaction for some tests
using (localOption $ Interactive true).  Instead, we turn it on in the
CI, breaking generability of haskell-ci.yml.
andreasabel added a commit that referenced this issue Sep 20, 2021
Use the abstractions from System.Process to invoke a shell; should be
more portable.
andreasabel added a commit that referenced this issue Sep 20, 2021
`callCommand "colordiff"` does not work under Cygwin because colordiff
is a python script and needs to be called via a shell that understands
shebangs (which CMD.EXE doesn't do, and the latter is likely behind
`callCommand`).
andreasabel added a commit that referenced this issue Sep 20, 2021
andreasabel added a commit that referenced this issue Sep 20, 2021
Automatically accepting tests in batch mode isn't a good idea, see:
#31 (comment)

So we revert to the previous behavior: interactive tests will fail if
EOI is reached.
@andreasabel
Copy link
Collaborator

Fix released in version 3.3.

@andreasabel andreasabel added this to the 3.3 milestone Sep 20, 2021
andreasabel added a commit to agda/agda that referenced this issue Nov 17, 2021
* [ fix #5302 ] make 'cabal test' work for part of the test-suite

'cabal test' will run only the tests defined by 'agda-tests'

* [ #5302 ] CI: work around haskell/cabal#7577

An attempt to get `cabal test` to run after *installing* Agda rather
than just building it.

* [ #5302 ] disable latex-tests for `cabal test`

Mainly for CI, so that we do not have to install LaTeX.

* [ #5302 ] disable compiler-stdlib tests for `cabal test`

Do not work out of the box, need installation of the stdlib (for
`Everything.agda`).

* [ #5302 ] CI: `cabal test` also on macOS and Windows

* [ #5302 ] `cabal test` macOS: work around haskell/cabal#7209

Cabal removes lower-case vowels from package names on macOS, see
haskell/cabal#7209.  Thus Agda might be just Agd in error messages
reporting source locations.

* [ #5302 ] giving up on `cabal test` under Windows

The testsuite fails but does not display diffs.
tasty-silver might not yet be portable, see

  phile314/tasty-silver#16

* [ #5302 ] fix the alert printed by `cabal test`

Correct reference is `v1-install` in `cabal v1-install --run-tests`.

* PR #5536: final polishing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants