-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Revert "Revert "Add regression tests on optimise"" #3164
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
Revert "Revert "Add regression tests on optimise"" #3164
Conversation
This reverts commit 1943f4e.
8948a38
to
ebc8e5d
Compare
ebc8e5d
to
b8c4b11
Compare
@@ -298,6 +294,13 @@ class CompilationTests extends ParallelTesting { | |||
tests.foreach(_.delete()) | |||
} | |||
|
|||
@Test def testOptimised: Unit = { | |||
val outputDir = defaultOutputDir + "optimised/" |
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.
outputDir
is never used
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.
supposed to be used in the tree lines bellow
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.
Fixed
b8c4b11
to
2caa7ba
Compare
val outputDir = defaultOutputDir + "optimised/" | ||
compileFilesInDir("../tests/pos", defaultOptimised, outputDir).checkCompile() | ||
compileFilesInDir("../tests/run", defaultOptimised, outputDir).checkRuns() | ||
compileShallowFilesInDir("../tests/neg", defaultOptimised, outputDir).checkExpectedErrors() |
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.
Should we move each test in its corresponding test group (i.e. compilePos
, compileNeg
, runAll
) to run them in parallel or should we keep them together for clarity?
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.
I would prefer to keep the separated for now because I going to start modifying stuff in the local optimizations. It will be clearer to see where the failures occurred if they happen. There are enough tests in each one to take advantage of parallelism, we can improve it later.
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.
Small nitpicks. Otherwise LGTM
This reverts commit 1943f4e.