Prevent real requests to the cloud during integration tests #2802
Conversation
na--
force-pushed
the
prevent-real-network-requests-in-tests
branch
from
4820177
to
f77f8e4
Compare
Codecov Report
@@ Coverage Diff @@
## master #2802 +/- ##
==========================================
- Coverage 76.21% 75.99% -0.23%
==========================================
Files 210 208 -2
Lines 16435 16430 -5
==========================================
- Hits 12526 12486 -40
- Misses 3146 3176 +30
- Partials 763 768 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| "reports.k6.io": true, | ||
| }, | ||
| } | ||
| http.DefaultTransport = bt |
There was a problem hiding this comment.
I know that this is not mandatory, but should we do a clean-up after the test run? 🤔
There was a problem hiding this comment.
Hmm I can add cleanup in a defer, but there is literally nothing after TestMain, the test exits. On the flip side, this is why we can also detect leaked goroutines in #2803
There was a problem hiding this comment.
Yes, I know. I was saying that http.DefaultTransport is a global package variable, and it's kind of the best practice to revert its state after the test run.
There was a problem hiding this comment.
In general I agree, but IIRC, Go compiles and tests different packages in isolation, so it shouldn't matter. But even if it didn't, I'd argue it's probably better to have this http.DefaultTransport for as many tests as possible, just in case. To intentionally not revert it back.
There is no way normal k6 behavior will be affected, only tests, and it will either break a problematic test or do nothing, so no downsides. I even wondered if I shouldn't also os.Setenv() a real HTTP_PROXY environment variable and implement a small blocking proxy, just in case some part of cmd/ uses a custom http.RoundTripper (that doesn't rely on http.DefaultTransport) in the future, but decided that might be a bit too far to go and not worth it just for a bit of extra assurance 😅
These integration tests might have been a little too real... 😅
The first commit masks a data race around stdout (only until #2800 is merged) and a flaky check. The second commit ensures integration tests don't make usage report requests and adds a check to hopefully prevent a similar mistake in the future.