-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
End to end tests #39
End to end tests #39
Conversation
I still have to add add some tests for binary and for bisecting. But please do review this PR meanwhile. |
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.
Great work!
I saw the TODO
s in the end-to-end test framework. Please add Haddocks to make the code more understandable.
0587f41
to
d74db04
Compare
a96cdcf
to
2986790
Compare
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.
Nice work once again. Looks good to me, but I still left some questions.
-- defined options. All the $$$OUT$$$ template value replaced the output file. | ||
-- Only one output file should be configured in the options as we can define | ||
-- only one expected output. The output file should be text file of some sort. | ||
evaluatePipelineTest input options expected ext params actionWith progressCallback = do |
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.
Why is the type signature missing? Is it too complicated?
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.
It is a local helper function called only once. If you are really think a type signature is necessary I'll add them, but at the time of the writing I didn't feel the need of adding the type signatures.
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.
In my opinion, it is always good to have type signatures for top-level functions. Makes the code more readable, makes the docs more understandable.
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.
Than I'll add that ugly ones :D
grin/src/Pipeline/Pipeline.hs
Outdated
@@ -257,6 +257,8 @@ data PipelineOpts = PipelineOpts | |||
, _poLintOnChange :: Bool | |||
, _poTypedLint :: Bool -- Run HPT before every lint | |||
, _poSaveBinary :: Bool | |||
, _poRuntimeC :: FilePath |
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.
Why are runtime.c
or primop.c
sources special?
I'd rater store a list of c source files in general.
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.
As we dedicated them to be special before. I also like the list idea better. Chaging...
grin/src/Pipeline/Pipeline.hs
Outdated
("clang-7 -O3 prim_ops.c runtime.c %s.o -s -o %s" ++ if debugSymbols then " -g" else "") | ||
grinOptCodeFile fname | ||
("clang-7 -O3 %s %s %s.o -s -o %s" ++ if debugSymbols then " -g" else "") | ||
(_poPrimOpsC cfg) (_poRuntimeC cfg) grinOptCodeFile fname |
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.
Use the list of c source files here.
Solves #38