Start to run setup() and teardown() only if they were defined
#2791
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 makes the tests easier to read and the init checks are important to test the current state, since that behavior will be changed in the next commit.
na--
force-pushed
the
run-setup-teardown-only-if-defined
branch
from
1a4f3fe to
876e817
Compare
olegbespalov
previously approved these changes
Dec 2, 2022
imiric
reviewed
Dec 2, 2022
Previous to this commit, k6 would initialize a transient VU and try to execute setup() and teardown() even if we knew they were not defined and exported by the script. That VU was created and then immediately discarded afterwards. It was probably done like that because, when setup() and teardown() were implemented in k6 v0.20.0, we didn't keep track of what JS functions were exported from the script. We only started doing that in k6 v0.27.0, when we added support for multiple scenarios, each with a potentially different `exec` function. We just didn't go back and improve the setup() and teardown() efficiency afterwards and they continued to use the "try to run it and see if it works" approach. While it was inefficient, it didn't use to be a big problem before. However, now that we want VU initialization to be interruptible by cancelling a context, it became a problem for fine-grained unit tests. Because the setupTimeout and teardownTimeout default values are defined in the cmd/ package, tests in sub-packages like js/ and core/ saw their values as 0. So, they created contexts with 0 timeouts, which immediately expired and the transient VUs for setup() and teardown() couldn't even be initialized without an error. The config mess strikes again... I thought about fixing the config somehow, but actually not running the setup() and teardown() functions if they were not defined seems both simpler and cleaner. Any test low-level that wants to use them can just manually define their timeouts, which is usually a good idea to do anyway.
na--
force-pushed
the
run-setup-teardown-only-if-defined
branch
from
876e817 to
10c7581
Compare
imiric
approved these changes
Dec 2, 2022
olegbespalov
approved these changes
Dec 2, 2022
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.
Previous to this PR, k6 would initialize a transient VU and try to execute
setup()andteardown()even if we knew they were not defined and exported by the script. That VU was created and then immediately discarded afterwards.It was probably done like that because, when
setup()andteardown()were implemented (#457) in k6 v0.20.0, we didn't keep track of what JS functions were exported from the script. We only started doing that in k6 v0.27.0, when we added support for multiple scenarios, each with a potentially differentexecfunction. We just didn't go back and improve thesetup()andteardown()efficiency afterwards and they continued to use the "try to run it and see if it works" approach.While it was inefficient, it didn't use to be a big problem before. However, now that we want VU initialization to be interruptible by cancelling a context (#2790), it became a problem for fine-grained unit tests. Because the
setupTimeoutandteardownTimeoutdefault values are defined in thecmd/package, tests in sub-packages likejs/andcore/saw their values as0😭 So, they created contexts with0timeouts, which immediately expired and the transient VUs forsetup()andteardown()couldn't even be initialized without an error. Our config mess (#883) strikes again...I thought about fixing the config somehow, but actually not running the
setup()andteardown()functions if they were not defined seems both simpler and cleaner. Any low-level test that wants to use them can just manually define their timeouts, which most do and is usually a good idea anyway (the default 60s value is usually too much for tests).