-
Notifications
You must be signed in to change notification settings - Fork 71
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
Introduce --test-plan
for more complex test plans
#422
Conversation
--test-plan
for more complex test plans--test-plan
for more complex test plans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested and I can confirm that it works as expected (with the current limitations). Great job! This is not a small task!
It would be awesome to have some more automated tests for this as we're dealing with quite significant change.
I would also propose to create a list of follow-ups that are required and mark those that are release blocker. This way we'll avoid forgetting to fix something crucial.
CHANGELOG.md
Outdated
@@ -13,6 +13,8 @@ | |||
- [#379](https://github.com/tag1consulting/goose/pull/379) **API change**: default to `INFO` level verbosity, introduce `-q` to reduce Goose verbosity | |||
o **note**: `-v` now sets Goose to `DEBUG` level verbosity which when enabled will negatively impact load test performance; set `-q` to restore previous level of verbosity | |||
- [#379](https://github.com/tag1consulting/goose/pull/379) **API change**: remove `.print()` which is no longer required to display metrics after a load test, disable with `--no-print-metrics` or `GooseDefault::NoPrintMetrics` | |||
- @TODO: introduce `--test-plan` and `GooseDefault::TestPlan` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR # missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
writeln!( | ||
fmt, | ||
" {:<12} {:<19} {:<17} {:<10} Users", | ||
"Action", "Started", "Stopped", "Elapsed", | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if this output really belongs in this file. Are these metrics or something else?
metrics.rs
is already a huge file and we might want to consider splitting it at some point? Maybe this is a good time to start this discussion (actually doing it is out of scope for this PR of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the ASCII display of the metrics. I suppose it could be moved into report.rs if that's a better fit?
src/config.rs
Outdated
pub(crate) struct TestPlan { | ||
// A test plan is a vector of tuples each indicating a # of users and milliseconds. | ||
pub(crate) steps: Vec<(usize, usize)>, | ||
// Which step of the test_plan is currently running. | ||
pub(crate) current: usize, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put test plan and its logic in a separate file. I would also include TestPlanStepAction
and TestPlanHistory
that are currently in metrics.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that I'd also add some test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's very little logic here, but I suppose it's likely going to grow -- splitting it out works for me. (Especially as when I was writing this I could never remember which file it lived in).
And yes, it DEFINITELY needs tests! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split it into its own file now -- definitely the right call, thanks.
ca46062
to
8dfb27d
Compare
Awesome work! 🎉 |
RTBM - retro — great work 👍 |
--test-plan
andGooseDefault::TestPlan
for configuring test plansFromStr
] to auto convert--test-plan "{users},{timespan};{users},{timespan}"
, where{users}
must be an integer, ie "100", and{timespan}
can be integer seconds or "30s", "20m", "3h", "1h30m", etc, to internalVec<(usize, usize)>
representation--test-plan
together with--users
,--startup-time
,--hatch-rate
or--run-time
From the new book page:
In the current implementation, a test plan can only Increase and Maintain, the load test will shut down any time it defines a Decrease. This will be resolved in a followup.
An example run:
% cargo run --release --example drupal_memcache -- --host http://apache.fosciana/ --test-plan "10,5;20,5;30,5;30,5;60,10;60,2;0,0"
Includes the follow overview in the metrics: