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

Resolve cargo check errors in benchmarks #3585

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

stjepangolemac
Copy link
Contributor

@stjepangolemac stjepangolemac commented Feb 24, 2023

Description

Prior to this PR we had 50-60 errors in the codebase mainly situated in the bench/ directory. They haven't been updated for a year or two. Deleting them doesn't seem like a good solution as we might want to use them in the future. Also, bringing them up to date seems like it wouldn't be a quick task.

For now, I've excluded them from the compilation by doing these two things:

  1. Renamed benchmark files to *.rs.backup
  2. Removed [[bench]] entries from Cargo.toml

Unrelated, but I've also removed two unused variables so we end up with no errors.

Applicable issues

@stjepangolemac stjepangolemac linked an issue Feb 24, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #3585 (24d064f) into master (58a376f) will decrease coverage by 5.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3585      +/-   ##
==========================================
- Coverage   31.16%   26.14%   -5.03%     
==========================================
  Files         297      297              
  Lines      274478   274476       -2     
==========================================
- Hits        85540    71753   -13787     
- Misses     188938   202723   +13785     
Impacted Files Coverage Δ
testnet/stacks-node/src/tests/epoch_21.rs 27.88% <100.00%> (-55.76%) ⬇️
testnet/stacks-node/src/monitoring/mod.rs 0.00% <0.00%> (-100.00%) ⬇️
src/cost_estimates/fee_medians.rs 0.00% <0.00%> (-92.38%) ⬇️
src/cost_estimates/fee_rate_fuzzer.rs 0.00% <0.00%> (-60.87%) ⬇️
testnet/stacks-node/src/tests/neon_integrations.rs 36.29% <0.00%> (-51.07%) ⬇️
src/chainstate/burn/operations/transfer_stx.rs 0.00% <0.00%> (-45.50%) ⬇️
testnet/stacks-node/src/tests/integrations.rs 0.00% <0.00%> (-40.34%) ⬇️
clarity/src/vm/analysis/mod.rs 37.00% <0.00%> (-39.00%) ⬇️
src/chainstate/coordinator/mod.rs 50.32% <0.00%> (-29.42%) ⬇️
... and 119 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stjepangolemac stjepangolemac marked this pull request as ready for review February 24, 2023 09:02
@kantai
Copy link
Member

kantai commented Feb 24, 2023

I'd be in favor of deleting any that don't compile, though I know that may be controversial.

The git history will keep that "deleted" data, so I'm not worried about data loss, and these files are not documentation, so I'm also not worried about breaking any internal links.

@kantai kantai changed the title Resolve cargo check errors Resolve cargo check errors in benchmarks Feb 24, 2023
@stjepangolemac
Copy link
Contributor Author

stjepangolemac commented Feb 27, 2023

My first thought was to delete this but wanted to first propose a less aggressive approach. @igorsyl would you be against deleting it?

@igorsyl
Copy link
Contributor

igorsyl commented Feb 27, 2023

Not at all. Like @kantai pointed out, git doesn't forget.

@stjepangolemac
Copy link
Contributor Author

Okay, I removed benchmark files and this can now be reviewed.

@jcnelson
Copy link
Member

Consensus is that it's fine to nuke this code.

Copy link
Contributor

@igorsyl igorsyl left a comment

Choose a reason for hiding this comment

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

image

@stjepangolemac
Copy link
Contributor Author

I'm not authorized to merge this one, @igorsyl who usually does that?

@igorsyl
Copy link
Contributor

igorsyl commented Mar 1, 2023

I'm not authorized to merge this one, @igorsyl who usually does that?

All CI tests are passing (except CodeCov), it's got two review approvals, and Jude expressed consensus support. I'm merging to unblock.

cc @wileyj for discussion later on how to handle merge access for collaborators.

@igorsyl igorsyl merged commit 07e7a61 into master Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Benchmarks are not up to date
4 participants