Skip to content

Delay printing playback harness until after verification status#2480

Merged
YoshikiTakashima merged 9 commits intomodel-checking:mainfrom
YoshikiTakashima:yoshi-2462-concrete-test-print-loc
May 27, 2023
Merged

Delay printing playback harness until after verification status#2480
YoshikiTakashima merged 9 commits intomodel-checking:mainfrom
YoshikiTakashima:yoshi-2462-concrete-test-print-loc

Conversation

@YoshikiTakashima
Copy link
Copy Markdown
Contributor

@YoshikiTakashima YoshikiTakashima commented May 26, 2023

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
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

YoshikiTakashima commented May 26, 2023

@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
Copy Markdown
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
Copy Markdown
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

Copy link
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concrete playback warnings / results printed in the middle of the verification report

3 participants