Repository navigation
Conversation
Replace the current set of Process constructors by non-throwing ones only and feed [(ExitCode, ByteString, ByteString)] to runPure. This enables it to mock all cases of runIO we care about. read_ becomes a function in Sem using Error ExitCodeException. This change doesn't affect existing code as nothing is currently using ourReadProcess_Sem, and NixpkgsReview.run uses ourReadProcessInterleavedSem which in turn uses ReadInterleaved as before. runPure will concatenate stdout and stderr for ReadInterleaved, which test authors need to be aware of.
4z3
changed the title
rhendric
requested changes
Oct 27, 2026
rhendric
left a comment
rhendric
left a comment
Member
There was a problem hiding this comment.
Good start! A few changes, please:
Comment on lines
+21
to
+24
| either | ||
| ((`shouldBe` ExitFailure 1) . TP.eceExitCode) | ||
| (expectationFailure . ("expected Left, but got Right " ++) . show) | ||
| result |
Member
There was a problem hiding this comment.
Suggested change
| either | |
| ((`shouldBe` ExitFailure 1) . TP.eceExitCode) | |
| (expectationFailure . ("expected Left, but got Right " ++) . show) | |
| result | |
| left TP.eceExitCode result `shouldBe` Left (ExitFailure 1) |
Comment on lines
+29
to
+32
| either | ||
| (expectationFailure . ("expected Right, but got Left " ++) . show) | ||
| (`shouldBe` (ExitSuccess, "foobar")) | ||
| result |
Member
There was a problem hiding this comment.
Suggested change
| either | |
| (expectationFailure . ("expected Right, but got Left " ++) . show) | |
| (`shouldBe` (ExitSuccess, "foobar")) | |
| result | |
| result `shouldBe` Right (ExitSuccess, "foobar") |
| result | ||
|
|
||
| describe "ReadInterleaved" do | ||
| it "concatenation stdout and stderr under runPure" do |
Member
There was a problem hiding this comment.
Nit:
Suggested change
| it "concatenation stdout and stderr under runPure" do | |
| it "concatenates stdout and stderr under runPure" do |
| return (r, "") | ||
| ReadInterleaved_ _config -> maybe "" id <$> input | ||
| Read _config -> | ||
| maybe (ExitSuccess, "", "") id <$> input |
Member
There was a problem hiding this comment.
Suggested change
| maybe (ExitSuccess, "", "") id <$> input | |
| fromMaybe (ExitSuccess, "", "") <$> input |
(This is the sort of tidying up that I encourage if you're going to be touching the line anyway.)
| ReadInterleaved _config -> do | ||
| r <- maybe "" id <$> input | ||
| return (ExitSuccess, r) | ||
| (exitCode, out, err) <- maybe (ExitSuccess, "", "") id <$> input |
Member
There was a problem hiding this comment.
Suggested change
| (exitCode, out, err) <- maybe (ExitSuccess, "", "") id <$> input | |
| (exitCode, out, err) <- fromMaybe (ExitSuccess, "", "") <$> input |
|
|
||
| ourReadProcess_Sem :: | ||
| (Members '[P.Process] r) => | ||
| (Members '[P.Process, Error ExitCodeException] r) => |
Member
There was a problem hiding this comment.
We probably want this to go through mapError tshow to become Error Text, to match the type of the ExceptT Text functions that this will replace. Error Text is a bit distasteful but matching the existing idiom is the first priority, so that we can establish tests and then refactor at will.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the current set of Process constructors by non-throwing ones only and feed [(ExitCode, ByteString, ByteString)] to runPure. This enables it to mock all cases of runIO we care about.
read_ becomes a function in Sem using Error ExitCodeException.
This change doesn't affect existing code as nothing is currently using ourReadProcess_Sem, and NixpkgsReview.run uses ourReadProcessInterleavedSem which in turn uses ReadInterleaved as before.
runPure will concatenate stdout and stderr for ReadInterleaved, which test authors need to be aware of. (True interleaving doesn't seem worth mocking, or rather might be mockable by using e.g. just stdout and leaving stderr as "".)
Tests have been added showing how read_ under runPure gives Left ExitCodeException, and how readInterleaved concatenates stdout and stderr.
This PR implements steps 1. through 3. of #585 (comment).