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.

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 coverage test suite #2608

Closed
wants to merge 6 commits into from
Closed

Conversation

jaisnan
Copy link
Contributor

@jaisnan jaisnan commented Jul 17, 2023

Description of changes:

Adds coverage test suite

Resolved issues:

Resolves #ISSUE-NUMBER

Related RFC:

Optional #ISSUE-NUMBER.

Call-outs:

Testing:

  • How is this change tested?

  • Is this a refactor change?

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.

Copy link
Contributor

@adpaco-aws adpaco-aws left a comment

Choose a reason for hiding this comment

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

Good work! A couple of high-level comments:

Why aren't filenames included in expected files? Looks like you're parsing file locations but only keep line numbers. Coverage data will give you information on external files (including Kani library or std functions), which we also need to store in order to make a correct comparison.

I'd also encourage you to use types to represent coverage data. That'd allow us to split this code into two parts: parsing and checking.

Comment on lines +587 to +590
// Iterate through the blocks and find the ones containing "UNREACHABLE" or "SATISFIED"
for block in blocks {
if block.contains("UNREACHABLE") || block.contains("SATISFIED") {
blocks_with_unreachable_or_satisfied.push(block);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be looking for UNSATISFIED and SATISFIED because those are the only two possible results for cover checks.

}
}
}
if block.contains("UNREACHABLE") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

@jaisnan jaisnan closed this Jul 19, 2023
@adpaco-aws adpaco-aws mentioned this pull request Jul 20, 2023
4 tasks
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.

2 participants