-
Notifications
You must be signed in to change notification settings - Fork 217
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
Swap order of waitForProcess and hGetContents to avoid blocking on windows #2101
Conversation
bors try |
Seeing some failing stake pool tests on windows, not sure if related or if they previously existed
|
tryBuild failed
|
bors try |
tryBuild succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good, let me know how the windows tests are reacting?
(WALLETS_UTXO_02 passes though, which was the goal of this PR) Trying to avoid a 60 minute feedback loop, I'm thinking I should try to better understand the windows process logic in isolation with a property test (seems good to have even if this PR didn't cause the stake pool test failures) |
Didn't manage to reproduce anything interesting using a
Using (actually, the above results are from another version, but approximately the same): let proc' cmd args = (proc cmd args)
{ std_in = CreatePipe, std_out = CreatePipe, std_err = CreatePipe }
let process = proc' ($(getTestData) </> "echoline.exe") []
let echoViaProcess x = withCreateProcess process $
\(Just stdin) (Just stdout) (Just stderr) h -> do
hPutStr stdin (x ++ "\n")
hFlush stdin
hClose stdin
c <- waitForProcess h
out <- TIO.hGetContents stdout
err <- TIO.hGetContents stderr
return (c, T.unpack out, err)
let assertEqual a b = do
monitor $ counterexample $ show a ++ " /= " ++ show b
assert $ a == b
it "alphaNum chars rountrip"
$ property
$ \(str :: String) -> monadicIO $ do
let a = filter isAlphaNum str
(_, b, _) <- run $ echoViaProcess a
let strip = T.unpack . T.strip . T.pack
strip b `assertEqual` a
|
I think merging and seeing what the windows CI says about this shouldn't hurt. |
This PR was included in a batch with a merge conflict, it will be automatically retried |
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed
|
bors r+ |
This PR was included in a batch with a merge conflict, it will be automatically retried |
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> 2105: Add test case for importing invalid addresses for Byron r=hasufell a=hasufell #2011 Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Julian Ospald <[email protected]>
This PR was included in a batch that was canceled, it will be automatically retried |
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> 2105: Add test case for importing invalid addresses for Byron r=hasufell a=hasufell #2011 Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Julian Ospald <[email protected]>
Build failed (retrying...)
|
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed
|
bors r+ |
Build failed |
P.S. You do not need to wait for the nightly build to check that it will work. |
That wasn't my problem. I was already running with Thanks for the Just swapping out the
|
bors r+ |
I was suddenly unable to reproduce the failing stake pool tests on windows, so I'm keen to have CI be running this. |
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed |
bors try |
Adding temporary code to run in |
tryBuild failed |
6f2b8ab
to
35a3b4d
Compare
bors try |
tryBuild succeeded |
17adb2a
to
bfeac1b
Compare
bors r+ |
2101: Swap order of waitForProcess and hGetContents to avoid blocking on windows r=Anviking a=Anviking # Issue Number #2083 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Swapping order of operations seems to fix problem that only arises on windows, and with tx >=5 outputs - [x] Fixes `WALLETS_UTXO_02` # Comments - [ ] I am re-running _all_ windows tests locally to be more sure <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed |
bors try |
tryTimed out |
Issue Number
#2083
Overview
WALLETS_UTXO_02
Comments