-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make handleSummary func timeout configurable #3977
base: master
Are you sure you want to change the base?
Make handleSummary func timeout configurable #3977
Conversation
e5db7b9
to
aa27c83
Compare
I'd make sure to update the docs as well if this change does move forward: https://grafana.com/docs/k6/latest/using-k6/k6-options/reference/ |
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.
Thank you for the PR 🙇
It seems to me like you are changing way too many tests and I expect there might be some problems with the propagation of the default.
Can you try to rever each of the test changes and see if you can reduce them? I haven't commented in each place.
It also will be nice to get a test case in https://github.com/grafana/k6/blob/master/cmd/config_consolidation_test.go#L146 as that is where consolidation is usually tested as well as defaults.
exports.options = { | ||
summaryTrendStats: %s, | ||
handleSummaryTimeout: '120s', | ||
}; |
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.
Do you really need to update all of those 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.
My understanding is that the defaults are set at the cmd
level. But, these unit tests are testing the functionality of these respective modules independent of the runtime defaults.
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.
@mstoykov : Friendly ping on this! 😄
Signed-off-by: Alex Johnson <alex.kattathra.johnson@gmail.com>
aa27c83
to
eb88857
Compare
What?
It makes the timeout for the
handleSummary
function configurableWhy?
To solve issue #3976. To allow for generating summary for long running tests
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
Closes #3976