-
Notifications
You must be signed in to change notification settings - Fork 265
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
Add progress option #5218
Add progress option #5218
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.
Thanks for implementing this! I have a couple of very small suggestions.
@@ -0,0 +1,19 @@ | |||
Verified 0/5 symbols. Waiting for Foo to verify. | |||
Verified part 1/3 of Foo, located at line 4, using 8,7E+002 resources |
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 seems like it may be locale-specific output, using commas in some locales and decimal points in others, so I imagine this test may fail in CI. Or does the E1
style specify commas?
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 imagine this test may fail in CI
Is that based on previous experience? Is there a locale difference between the platforms? Seems like it would be better if we avoid this issue for all tests, instead of me adding some feature or test specific fix.
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.
Update: changed the feature to ignore the locale
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.
Oh, yeah, that's all I was suggesting: ignoring the locale. :)
Co-authored-by: Aaron Tomb <aarotomb@amazon.com>
…ny into verificationProgress
@atomb, could you help me understand why the line coverage rate for DafnyCore dropped by this PR (below the allowed threshold), even though the new code is being covered by the added littish test? |
I noticed this yesterday evening, and I'm similarly mystified. The test you added really should cover every single line of the code you added. One thing I notice is that it's exactly at the threshold. It says 86% line coverage achieved, and the lower bound is 86%. I'm guessing that it's rounding up from 85.something. It's possible that it was just barely at the threshold before your commit and something like rounding error was enough to make it switch from one side to the other? |
Yeah, I just did the actual division and it's 85.6%. The last merged PR had test coverage results of 86.4%, with a larger overall numerator but only a very slightly smaller denominator. I have a suspicion that just merging with |
That worked, thanks! |
Fixes #5150
Description
Add a
--progress
option to Dafny, that displays output like the following:Note that when
--cores
is more than 1, which is the default, the output will get more messy because "Verify x/y symbols" will be interspersed with "Verified part a/b of X"The locations of parts can be confusing, sometimes they point to the header of the symbol being verified, such as when verifying wellformedness of the symbol contract, or correctness when no
--isolate-assertions
is used. When--isolate-assertions
is used, they may also point to lines containing assertions.How has this been tested?
Add a littish test,
progress.dfy
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.