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 skips and comments for certain mutants #4604

Closed

Conversation

@ASuciuX ASuciuX marked this pull request as draft March 28, 2024 14:44
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (f9e9791) to head (dbacf43).
Report is 9 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4604      +/-   ##
==========================================
- Coverage   83.58%   83.18%   -0.41%     
==========================================
  Files         471      465       -6     
  Lines      337958   330892    -7066     
  Branches      317        0     -317     
==========================================
- Hits       282486   275248    -7238     
- Misses      55464    55644     +180     
+ Partials        8        0       -8     
Files Coverage Δ
stackslib/src/burnchains/db.rs 80.64% <ø> (-0.02%) ⬇️
stackslib/src/burnchains/mod.rs 82.64% <ø> (-0.09%) ⬇️
stackslib/src/chainstate/burn/db/sortdb.rs 91.90% <ø> (-0.04%) ⬇️
stackslib/src/chainstate/coordinator/mod.rs 62.06% <ø> (-2.63%) ⬇️
stackslib/src/chainstate/stacks/boot/mod.rs 94.74% <ø> (-0.02%) ⬇️
stackslib/src/chainstate/stacks/db/blocks.rs 89.84% <ø> (-0.24%) ⬇️
stackslib/src/chainstate/stacks/miner.rs 81.43% <ø> (+0.27%) ⬆️
stackslib/src/main.rs 0.06% <ø> (ø)
...-node/src/burnchains/bitcoin_regtest_controller.rs 87.54% <ø> (-0.64%) ⬇️
testnet/stacks-node/src/config.rs 65.48% <ø> (-0.42%) ⬇️
... and 3 more

... and 355 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9e9791...dbacf43. Read the comment docs.

@ASuciuX ASuciuX self-assigned this Apr 3, 2024
@ASuciuX ASuciuX changed the title [WIP] add skips and comments for fixing the tests for functions that were highlight in previous mutants CI runs Add skips and comments for fixing the tests for functions from PRs that were highlight in previous mutants CI runs Apr 3, 2024
@ASuciuX ASuciuX marked this pull request as ready for review April 3, 2024 16:35
@ASuciuX
Copy link
Contributor Author

ASuciuX commented Apr 11, 2024

This follows the same structure as #4593 which was previously merged. It includes the mutants for multiple PRs.

@ASuciuX ASuciuX requested review from jbencin and a team April 11, 2024 13:16
@ASuciuX ASuciuX changed the title Add skips and comments for fixing the tests for functions from PRs that were highlight in previous mutants CI runs Add skips and comments for certain mutants Apr 11, 2024
Copy link
Contributor

@jbencin jbencin left a comment

Choose a reason for hiding this comment

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

I think the associated comments should be regular comments, not doc comments. cargo doc is great for figuring out how to use an API, and these comments don't help anyone actually use the functions

@ASuciuX
Copy link
Contributor Author

ASuciuX commented Apr 11, 2024

I think the associated comments should be regular comments, not doc comments. cargo doc is great for figuring out how to use an API, and these comments don't help anyone actually use the functions

Those comments act as placeholders for missing tests, they should be reference for who wants to write the test cases that are in the comments and aren't treated by the functions. There should be something on the code to point to the specific missing cases so that a developer could know which test cases to add. Would it be enough to change them to simple comments instead of doc comments? Or do you think we should move them somewhere externally but also keep a short reference so that the cases can be seen when going to that external source. Also curious what that external source should be and how should I give access to the others to modify it if some of the tests are added.

@jbencin
Copy link
Contributor

jbencin commented Apr 12, 2024

Would it be enough to change them to simple comments instead of doc comments?

This is fine by me. Although you should add a second reviewer on here for another opinion

@ASuciuX
Copy link
Contributor Author

ASuciuX commented Apr 12, 2024

Would it be enough to change them to simple comments instead of doc comments?

This is fine by me. Although you should add a second reviewer on here for another opinion

I've added the blockchain team, but it treated your review in that behalf, so I'll add it again.

@ASuciuX ASuciuX requested a review from a team April 12, 2024 15:39
@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 7, 2024

@jbencin I've updated this to have regular comments

@ASuciuX ASuciuX requested a review from jbencin May 7, 2024 21:02
@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 9, 2024

As this is outdated and for next branch, I will move it to develop.

@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 14, 2024

Migrated the PR here #4790.

@ASuciuX ASuciuX closed this May 14, 2024
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants