Warn on shorthand options overriding scenarios #4176
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is currently implemented only for scenarios as that is the case that is most confusing. Implementing this for duration overriding stages (for example) is possible, but it seems to not be nearly hit enough. Unfortunately due to too many of things of #883 and the way shorthand options are handled in particular, the code can't be in any Apply function with enough information to have a nice message. Fixes #2529
mstoykov
requested review from
olegbespalov and
joanlopez
and removed request for
a team
olegbespalov
approved these changes
Jan 16, 2025
| if a.Scenarios != nil && | ||
| (b.Duration.Valid || b.Iterations.Valid || b.Stages != nil || b.Scenarios != nil) { | ||
| logger.Warnf( | ||
| "%q level configuration overrode scenarios configuration entirely", |
There was a problem hiding this comment.
Maybe explicitly mention implications, what do you think?
Suggested change
| "%q level configuration overrode scenarios configuration entirely", | |
| "%q level configuration overrode scenarios configuration entirely. It's highly likely, that the test load result will be different from what you expect.", |
There was a problem hiding this comment.
This usually isn't the problem - the problem is people expect:
- to run their scenarios but with a different duration, iterations or VUs, but with all other options the same
- having some other option stay the same - for example
exec,env, and alsooptionsfor browser
But making the message a bunch of sentences IMO isn't a great idea. The one we have with docker and stuff about not finding files - it still gets users to post it and ask questions. I only expect this is happening less because of google actually now fidning the results from previous asks.
joanlopez
approved these changes
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Warn on shorthand options(vus, duration, stages, iterations) overriding scenarios from previously defined scenarios. Mostly applicable when using cli flags to override them from within the script.
This is currently implemented only for scenarios as that is the case that is most confusing. Implementing this for duration overriding stages (for example) is possible, but it seems to not be nearly hit enough.
Unfortunately due to too many of things of #883 and the way shorthand options are handled in particular, the code can't be in any Apply function with enough information to have a nice message.
Why?
This is probably one of the things that confuses the most users as explained in the original #2529 issue.
Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Fixes #2529