-
Notifications
You must be signed in to change notification settings - Fork 847
Cleanup integration tests for stack repl
#6740
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
Conversation
c63aeb3
to
07c2223
Compare
stack repl
stack repl
Hi @mpilgrem, this is ready for review, please take a look. |
You'll see from the 'linting' CI that Hlint has a suggestion. There are exceptions (see The checking-in of the three |
@ulidtko, thanks. Looks good to me. You've not allowed edits from maintainers, otherwise I would have put through the small stuff/rebased directly. I've made some observations. |
runRepl was missing the `hClose rStdin` call -- causing the ghci subprocess to terminate abnormally with EPIPE on its stdin read.
@mpilgrem comments addressed & rebased. Maintainer edits allowed, too. |
Overhaul of integration tests that exercise
stack repl
, gets them to run, and pass, on all matrix platforms (windows, too). Resolves #6170.Glossing over various cleanups and refactorings (full details in commit messages), highlights of changes:
threadDelay
-driven "synchronization" in test logic;-- TODO: Understand why the exit code is 1 despite running GHCi tests successfully
;stderr
on exceptions in test code — resolving a visibility issue / improving debuggability;-ignore-dot-ghci
— its absence was causing failures in local runs on my machine.Removed stubs that disable repl tests on windows, introduced in the wake of #6170 (PR #6174). They run again.
Test scenarios themselves kept intact; besides simplifications and cleanups, no logical change in asserts.
Tested via local runs, and CI spins in my fork. I got 5 consecutive runs 🟢 passing all 5 matrix jobs, on the same code (modulo rebases over updates in master). The repl tests are definitely running, and apparently stable.
Checklist:
tests/
, no need