Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Delay printing playback harness until after verification status #2480

Merged
merged 9 commits into from
May 27, 2023
Merged

Delay printing playback harness until after verification status #2480

merged 9 commits into from
May 27, 2023

Conversation

YoshikiTakashima
Copy link
Contributor

@YoshikiTakashima YoshikiTakashima commented May 26, 2023 •
edited
Loading

Description of changes:

Delay printing of harness until the verification status message is printed.

Resolved issues:

Resolves #2462

Call-outs:

Again, not familiar with testing this feature, so feel free to suggest any improvements on that front.

Testing:

  • How is this change tested? tests/ui/concrete-playback/message-order

  • Is this a refactor change? No

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@YoshikiTakashima YoshikiTakashima requested a review from a team as a code owner May 26, 2023 19:36
@YoshikiTakashima YoshikiTakashima changed the title Yoshi 2462 concrete test print loc Delay printing playback harness until after verification status May 26, 2023
jaisnan
jaisnan reviewed May 26, 2023
View reviewed changes
Copy link
Contributor

@jaisnan jaisnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried running concrete playback with --inplace. Does the behavior of pasting the unit test into the source get impacted by this change? Even if it's not, I think it would be good to have a few regression tests with inplace just so we know the impact.

@YoshikiTakashima
Copy link
Contributor Author

Good pt. I don't think so, but let me write another test to be sure.

celinval
celinval requested changes May 26, 2023
View reviewed changes
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest a different approach? Can you move the call to gen_and_add_concrete_playback() to harness_runner module instead of call_cbmc. We need to make sure that call is made after the result was rendered.

@YoshikiTakashima
Copy link
Contributor Author

YoshikiTakashima commented May 26, 2023 •
edited
Loading

@celinval I can do that. The inputs to gen_and_add_concrete_playback() will need to move into the VerificationResults to be available but that might be cleaner.?

@YoshikiTakashima
Copy link
Contributor Author

@celinval thanks for the suggestion. This was way easier, no change to VerificationResult required.

@YoshikiTakashima YoshikiTakashima enabled auto-merge (squash) May 26, 2023 23:03
@YoshikiTakashima
Copy link
Contributor Author

Should be good to go.

@jaisnan The similar tests to what you requested seem to have merged as part of #2464 and #2477

celinval
celinval approved these changes May 27, 2023
View reviewed changes
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks

@YoshikiTakashima YoshikiTakashima merged commit d4f0dd0 into model-checking:main May 27, 2023
@YoshikiTakashima YoshikiTakashima deleted the yoshi-2462-concrete-test-print-loc branch May 30, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaisnan jaisnan jaisnan left review comments

@celinval celinval celinval approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Concrete playback warnings / results printed in the middle of the verification report
3 participants
@YoshikiTakashima @celinval @jaisnan