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

Cover loadtest bin with tests #36

Closed
wants to merge 10 commits into from

Conversation

rustworthy
Copy link
Collaborator

@rustworthy rustworthy commented Dec 2, 2023

  • Refactoring loadtest binary to make it testable;
  • Adding tests for loadtest bin to increase overall coverage;

Loadtest functions as expected:
image

Coverage report for loadtest.rs (with suggested changes applied):
image

Overall ~10p.p. coverage increase from 69.88% to 79.46%:
image


This change is Reviewable

@rustworthy rustworthy changed the title [WIP] Cover loadtest bin with tests Cover loadtest bin with tests Dec 2, 2023
@jonhoo
Copy link
Owner

jonhoo commented Dec 17, 2023

I'm not sure I follow the reasoning behind this change. loadtest is intended to be a simple program for running load testing against Faktory, and I don't think it's particularly important that it itself be tested (beyond the fact that it compiles). I'd be more inclined to simply exclude it from the coverage calculation if we're worried about it artificially dragging the number down.

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Merging #36 (b695db4) into main (b63eadf) will increase coverage by 9.3%.
Report is 2 commits behind head on main.
The diff coverage is 94.8%.

Additional details and impacted files
Files Coverage Δ
src/bin/loadtest.rs 95.4% <94.8%> (+94.3%) ⬆️

... and 1 file with indirect coverage changes

@rustworthy
Copy link
Collaborator Author

I'm not sure I follow the reasoning behind this change. loadtest is intended to be a simple program for running load testing against Faktory, and I don't think it's particularly important that it itself be tested (beyond the fact that it compiles). I'd be more inclined to simply exclude it from the coverage calculation if we're worried about it artificially dragging the number down.

I believe it's easier to read and reason about when it's decomposed. Say, we will want to load test with some of new features in place: job tracker, who will send a job execution updates once the job is fetched. Then it will be easier to adjust the binary to accept these options from the command line. The fact (fakt) that we can now cover these separate procedures with tests is rather a side-effect, isn't it? :)

@jonhoo
Copy link
Owner

jonhoo commented Dec 17, 2023

Hmm, I actually find the new version significantly harder to read/follow than the old version 😅 I'd prefer to keep the old one until such time as we need the added flexibility of the more convoluted new one :)

@jonhoo jonhoo closed this Dec 17, 2023
@rustworthy rustworthy deleted the chore/loadtest-coverage branch February 21, 2024 14:43
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