chore: reduce time spent in Quarkus tests #1076
Merged
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
gsmet
added a commit
to gsmet/sbomer
that referenced
this pull request
Dec 23, 2024
While I agree you should stay compatible with the minor request by the Operator SDK extension, I don't think it's a good idea to stay on the micro as you are missing all the fixes we are pushing since at the moment the version used here is 3.17.0. So my advice is: update to the latest micro of the minor branch requested for the Operator SDK. This is important for project-ncl#1076 as otherwise, you will have to wait a lot longer to get the fix. Also it's good practice in general.
gsmet
added a commit
to gsmet/sbomer
that referenced
this pull request
Dec 23, 2024
While I agree you should stay compatible with the minor request by the Operator SDK extension, I don't think it's a good idea to stay on the micro as you are missing all the fixes we are pushing since at the moment the version used here is 3.17.0. So my advice is: update to the latest micro of the minor branch requested for the Operator SDK. This is important for project-ncl#1076 as otherwise, you will have to wait a lot longer to get the fix. Also it's good practice in general.
gsmet
added a commit
to gsmet/sbomer
that referenced
this pull request
Dec 23, 2024
While I agree you should stay compatible with the minor requested by the Operator SDK extension, I don't think it's a good idea to stay on the micro as you are missing all the fixes we are pushing since at the moment the version used here is 3.17.0. So my advice is: update to the latest micro of the minor branch requested for the Operator SDK. This is important for project-ncl#1076 as otherwise, you will have to wait a lot longer to get the fix. Also it's good practice in general.
|
FWIW I also created fabric8io/kubernetes-client#6750 . I worked around the issue but it would be nice to have a proper fix. |
gsmet
changed the title
gsmet
added a commit
to gsmet/sbomer
that referenced
this pull request
Dec 24, 2024
While I agree you should stay compatible with the minor requested by the Operator SDK extension, I don't think it's a good idea to stay on the micro as you are missing all the fixes we are pushing since at the moment the version used here is 3.17.0. So my advice is: update to the latest micro of the minor branch requested for the Operator SDK. This is important for project-ncl#1076 as otherwise, you will have to wait a lot longer to get the fix. Also it's good practice in general.
Some background: Quarkus is restarted whenever a test resource is restricted to the class or different from the ones from other tests and also when the test profile is different. There is a clever orderer in Quarkus that tries to order things cleverly to minimize the number of restarts but... ITs and Failsafe are not honoring the JUnit test orderer so you lose this benefit and you end up with Quarkus being restarted a lot more often than strictly necessary. So the first fix here is to consider the Quarkus tests as Surefire tests, not failsafe tests. If more practical for you, they can be isolated in a specific Surefire execution but we should keep them executed with Surefire. Second issue is that @WithKubernetesTestServer is restricted to a given test clean, meaning every time you have this annotation, the server will have to be restarted. I will start discussions to see if we should change that but in the meantime I added an annotation that has a less aggressive behavior. Unfortunately, you are also affected by a Quarkus bug, for which I will push a fix soon and that we will hopefully backport to the next 3.17.
That means that the tests are not isolated from this POV but Marek was saying this would be OK.
gsmet
force-pushed
the
improve-unit-testing
branch
from
27b405f to
c54a484
Compare
gsmet
commented
Dec 24, 2024
| assertNull(mockServer.getLastRequest()); | ||
| // there is no proper way to reset the last request at the beginning of the method | ||
| // so let's just compare it's the same as before and no requests have been executed. | ||
| assertSame(lastRequestPriorToExecution, mockServer.getLastRequest()); |
There was a problem hiding this comment.
I created fabric8io/kubernetes-client#6750 for that.
goldmann
pushed a commit
that referenced
this pull request
Jan 8, 2025
While I agree you should stay compatible with the minor requested by the Operator SDK extension, I don't think it's a good idea to stay on the micro as you are missing all the fixes we are pushing since at the moment the version used here is 3.17.0. So my advice is: update to the latest micro of the minor branch requested for the Operator SDK. This is important for #1076 as otherwise, you will have to wait a lot longer to get the fix. Also it's good practice in general.
|
FYI, I'm currently releasing Quarkus 3.17.6 that will come with the Quarkus fix that should improve things for you even more with this patch. I'll include the upgrade in this PR if not merged by then. |
|
Thanks for heads-up! I actually plan to merge this one now. I can do the Quarkus upgrade myself once it becomes available, or, if you are so kind, you could create a new PR. Hope this works for you. @gsmet Thanks for the support! |
|
Works for me, I'll create a PR once 3.17.6 has been released. |
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.
This is a follow up of Marek asking for help here: https://quarkusio.zulipchat.com/#narrow/channel/187030-users/topic/.40QuarkusTest.20best.20practices.20.28multiple.20tests.2C.20slowness.29
From the first commit comment (the second one is just going a step further):
Combined with quarkusio/quarkus#45253 , I was able to reduce the time spent in
servicemodule testing from4mn30to1m45and it should scale a lot better if you don't multiply the combination of resources + test profiles.There's one VERY important caveat though: the Kubernetes mock is shared with all the tests so you need to have this limitation in mind. Marek told me that was acceptable so here we are.