-
Notifications
You must be signed in to change notification settings - Fork 262
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
Exposing --output and --spill-translation in dafny test, per Issue 3612 #3769
Conversation
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.
Overall I think this looks good, but there are two CI failures, one trivial and one I'm not entirely sure the source of.
var regex = program.Options.MethodsToTest; | ||
if (regex != null) { | ||
string name = method.FullDafnyName; | ||
if (!Regex.Match(name, regex).Success) continue; |
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.
This should have braces, as the CI complains about.
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
if (hasMain) { | ||
Reporter.Error(MessageSource.Rewriter, mainMethod.tok, "Cannot use /runAllTests on a program with a main method"); | ||
return; | ||
} |
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 code at the end of this file still calls hasMain
and an assertion fails when running with /testContracts
. I wonder whether the implementation of HasMain
need to be updated to account for the possibility of the new synthetic name you're using?
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.
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.
That was a satisfyingly simple fix. :)
…-3612-test-command
…afny into cok-3612-test-command
Hey, I feel like a bit of a broken record, but adding options to the CLI is a one-way door, so we should be careful about what we add. @davidcok did we consider alternatives to this option? How do you feel about the design sketched in #3612 (comment) ? |
Fixes #3612
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.