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.

Sign up for GitHub

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

Jump to bottom

Windows portability (fix #16) #31

Merged
merged 22 commits into from
Sep 20, 2021
Merged

Windows portability (fix #16) #31

merged 22 commits into from
Sep 20, 2021

Conversation

andreasabel
Copy link
Collaborator

@andreasabel andreasabel commented Sep 6, 2021 •
edited
Loading

Windows portability (fix #16).

See README and CHANGELOG for a high-level description of changes.

I am planning to release this as 3.3 (major version), since the logic of interactive running without stdin has changes (assumes "yes" now).

@andreasabel andreasabel force-pushed the issue16 branch 2 times, most recently from 6c9bd92 to 4bd92ee Compare September 9, 2021 08:47
@andreasabel andreasabel changed the title Testcase for #16 Windows portability (fix #16) Sep 13, 2021
@andreasabel
Testcase for #16
2ddb9bd
@andreasabel
WIP testcase for #16: use interactive test runner
f861d2c
@andreasabel
WIP #16: print output of showDiff test
d557642
@andreasabel
WIP #16: E.catch, show test details direct on macOS/windows
ec378de
@andreasabel
Re #16: on git diff, interpret non-empty stderr as failure
6b2c53e
@andreasabel
Re #16: try shellEscape hack for Windows sh -c
bf66105
@andreasabel
Interactive: simplify tryAccept (fewer arguments)
dde3cfa
@andreasabel
Cosmetics
aa5b3a9
@andreasabel
Tests: use withCurrentDirectory to not affect subsequent tests
86de0e8 
setCurrentDirectory is persistent
@andreasabel
WIP #16: an interactive test where the golden value never matches
5047bdb 
This is WIP for debugging purposes, requires the self-tests to be run
in interactive mode.
@andreasabel
Re #16: replace shellEscape hack by backslashes-to-slashes
9c3c7b8 
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
Cosmetics; tryAccept: rename isTerm to isANSI
1fd573e
@andreasabel
Re #16: unify printDiff and showDiff; check for sh, git etc before ca…
0bf8aab 
…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
CI #16: add --test-options=-i (work around #32
5c2cbe7 
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
Re #16: replace "sh -c" by callCommand, shell, system, rawSystem
7e23839 
Use the abstractions from System.Process to invoke a shell; should be
more portable.
@andreasabel
Re #16: retain sh -c indirection for colordiff
d223299 
`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
CHANGELOG: added release dates (source: git tags)
e771a23
@andreasabel
README and CHANGELOG for #16
ad44d4c
@andreasabel andreasabel added this to the 3.3 milestone Sep 13, 2021
@andreasabel andreasabel self-assigned this Sep 13, 2021
@andreasabel andreasabel marked this pull request as ready for review September 13, 2021 10:37
phile314
phile314 approved these changes Sep 14, 2021
View reviewed changes
Copy link
Owner

@phile314 phile314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

I guess it currently returns an exit code of 0 if interactive mode updates a golden value automatically when there is no terminal. I would slightly prefer to return a non-0 exit code, but using interactive mode without a terminal is not really how it is intended to be used in the first place so I don't feel too strongly about that.

And I agree, bumping to version 3.3 is warranted due to the change in behavior.

@andreasabel
Copy link
Collaborator Author

@phile314 :

I guess it currently returns an exit code of 0 if interactive mode updates a golden value automatically when there is no terminal. I would slightly prefer to return a non-0 exit code, but using interactive mode without a terminal is not really how it is intended

Mmh, you have some point here. Maybe my design was too much influenced by my wish to test tasty-silver interactive mode using CI. To this end, there needs to be a test case that fails in order to show the diff to the golden value as if asking the user, but we do not want the CI to fail subsequently.

What we really want is to pass the --accept flag to such self-tests, but I am hampered by #32 (and maybe #24) here.

The logic implemented in this PR is not good, I see this now. E.g., in Agda we run the CI in interactive mode with a non-interactive terminal, and we do not want that broken tests get automatically accepted.

I wonder how to proceed this PR. Maybe it is best to take out the "accept if non-interactive terminal" logic and the respective self-test for now and otherwise merge the PR and release.
Then, given time, I can revisit the problem of self-testing the interactive mode. Maybe after solving #32 (or #24).

@andreasabel andreasabel marked this pull request as draft September 15, 2021 12:34
@phile314
Copy link
Owner

I wonder how to proceed this PR. Maybe it is best to take out the "accept if non-interactive terminal" logic and the respective self-test for now and otherwise merge the PR and release.

Sounds good to me 👍

@andreasabel
Re #16: revert to: don't accept automatically if -i < /dev/null
b5e1ba5 
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 Author

Pushed a commit that reverts back to the old logic for interactive mode (end-of-input means don't-update-golden-value).

@andreasabel andreasabel marked this pull request as ready for review September 18, 2021 16:10
@andreasabel
Copy link
Collaborator Author

Bumped to 3.3. Release candidate at: https://hackage.haskell.org/package/tasty-silver-3.3/candidate

@andreasabel
Copy link
Collaborator Author

Published! https://hackage.haskell.org/package/tasty-silver-3.3

@andreasabel andreasabel merged commit b9e7b54 into master Sep 20, 2021
andreasabel added a commit that referenced this pull request Sep 20, 2021
@andreasabel
Re #16: revert to: don't accept automatically if -i < /dev/null
5c1e37f 
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 andreasabel deleted the issue16 branch September 20, 2021 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phile314 phile314 phile314 approved these changes

Assignees

@andreasabel andreasabel

Labels
platform: windows
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.

Paths on windows get mangled
2 participants
@andreasabel @phile314