Move integration tests to a dedicated package #2459
Comments
na--
added
tests
refactor
high prio
evaluation needed
|
Yeah, I think we should leave it for the next release, possibly start this cooldown. I think it's relatively urgent that we do it because a whole bunch of not-quite-ok integration tests will start piling up until we do, and so I added the |
|
@na-- I started looking into this, and have a couple of questions:
|
Not quite - we'd will definitely need to export
Even if we export That said, if we need to, I don't even see a problem with having the It is a balance between cleanliness and usability, but our goal is not to completely prevent such things. We need to make it easy to use these mock-able objects and to make it obvious that it's a bad idea to mess with the OS and the external environment directly, that all interactions have to pass through these objects. However, nothing is stopping any k6 module (or an xk6 extension) to directly import the
Nothing, only integration tests. That is, these tests that essentially start from the root of k6 and run an almost version of the code. I think the only part of k6 that is not touched by the integration tests is However, some currently existing "integration tests" that are outside of the main And, crucially, they don't need any of these internal implementation details to actually verify whatever they are testing. They are just forced to re-implement them because they don't have access to the
See ⬆️, some integration tests actually need to be in a separate package. But we might want to move even the ones that are fine in |
No, I meant, what other integration tests that are not related to
Ah I see, but these tests are lower level than So I don't see this as boilerplate. It's more of the required test setup for the level they're written in. If we had a need for more tests at this level, then it would make sense to abstract this away. If not, as you seem to imply, and it only adds maintenance headaches, then sure, let's convert them to
Got it. Makes sense. The reason I'm reluctant to move I'll experiment a bit with this, but my rough plan so far is to:
Does this sound good? |
What I was trying to say is that these tests are currently lower level, but they didn't need to be lower level. They were forced to be white-boxy because they didn't have access to the
I somewhat disagree, it's not very specific to It just so happens that the majority of The only reason not to do something like this would be to gently encourage lower-level components inside of k6 to avoid directly querying the "raw" environment and use other finer-grained abstractions instead. For example, instead of reading an env variable, to query the value from the If we want to go to extremes, I'd definitely prefer to have the whole
Hmm I kind of think it will be simpler to just have Similarly, |
|
Hey, I'm done with most of the changes we agreed on, but have a question about which tests should be moved to I moved all of the ones in I'm inclined to say "yes", and to consider an integration test any test that runs EDIT 2022-12-16: I opted to keep these where they are (see #2821), since it would mean most of the |
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
This allows us to use it in tests outside of the cmd package. See #2459
This also removes the duplication of the xk6-timers module, and uses it directly now that it's an experimental built-in module. Although, shouldn't these be xk6-timers tests rather than event loop ones? We have plenty of tests that indirectly test the event loop, and we could always add much simpler ones that don't involve the timers module. See #2459
#2412 and its preceding PRs refactored some cruft in
cmd/and allowed us to safely and easily write full integration tests of k6. However, these tests are limited tocmd/, since theglobalTestStateandglobalStatetypes, as well as the helper functions and other types they depend on are not exported:k6/cmd/root_test.go
Lines 18 to 30 in 6d7d732
As evidenced by #2399 (comment), it would be much better to further refactor
cmd/, to export and maybe even move these types out of there into a new package or packages, so we can have a dedicated top-leveltests/orintegration_tests/package where we could organize these tests nicely.The text was updated successfully, but these errors were encountered: