-
Notifications
You must be signed in to change notification settings - Fork 594
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
feat(tests): isolated integration tests #4823
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4823 +/- ##
=======================================
- Coverage 77.9% 76.8% -1.2%
=======================================
Files 168 170 +2
Lines 18899 19050 +151
=======================================
- Hits 14736 14637 -99
- Misses 3336 3583 +247
- Partials 827 830 +3 ☔ View full report in Codecov by Sentry. |
1b24e8c
to
3125c2f
Compare
4b2a97b
to
1bbbda0
Compare
1bbbda0
to
2062698
Compare
c19c20e
to
bc288cb
Compare
6bccf5e
to
51364b4
Compare
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.
🎖️ I like the idea very much, especially the structure it can enforce and the flexibility it gives us in terms of injecting various dependencies into a test body. That promotes writing concise and well-readable test cases (or at least makes that easier).
I took a first pass of the review. Will take another tomorrow. 🖖
b818ba2
to
f752a6f
Compare
f752a6f
to
1487dbe
Compare
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.
This looks great, @pmalek!
I have one question though - we are running in isolation the UDPRoute tests only. Which other tests do you think we can run in such an isolated environment? Do we have something we should avoid?
I don't have everything written down because at the time I put up this PR I wasn't sure everyone would be on board with this approach but hey! It seems now that we're mostly OK with this we can plan ahead. My very high level plan for this is:
|
I like this plan, sounds good to me 👍 |
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.
🚀
@randmonkey and @programmer04 Feel free to respond to the still opened conversations and close them if you feel they have been addressed. |
Co-authored-by: Jakub Warczarek <[email protected]>
24da4ba
#4847 can be considered a follow up to this PR. When this gets merged we're already going to use a version of e2e-framework which will contain the fix. Another follow up could be #4849 which removes the hardcoded FF and would make tests read those from env (as it's currently the case in the standard integration test suite). |
What this PR does / why we need it:
This PR tries to introduce a concept of isolated integration tests. The basic principles of those are:
Feature
s (more details in updated TESTING.md) setup and teardownFeature
's labels we're able to filter the tests not only via names but also by labels and only those tests that were filtered will have their setup runFeature
s isBeforeEachFeature
andAfterEachFeature
are not skipped even though feature labels indicate so kubernetes-sigs/e2e-framework#330 which for some reason doesn't allow use to use a shared setup liketenv.BeforeEach[Test|Feature]
Feature
labels we'll be able to use a shared kong instance (installed once inkong
namespace) or a dedicated one (installed perFeature
in its own dedicated namespace)Special notes for your reviewer:
Added a separate section in TESTING.md
So far the new suite is in the
test/integration/isolated
.What do we gain by this change in terms of numbers?
Non scientific comparison of relevant test suites:
NOTE: this moved the tests around, from the above mentioned suite, to
isolated-dbless-expression-router
suite (which run for: 6m0s) but the change is that in the isolated suite tests can run in parallel without any interference so the more tests we add there the more benefits we'll see.