Should we reconsider @WithTestResource?

[Guillaume Smet]

Hi,

I have been pondering sending this email for a week now as I gave a lot of thoughts to our journey fixing various issues causing OOM (see https://github.com/quarkusio/quarkus/issues/42471).

It is not an easy email to send and I sincerely hope someone will just explain to me how I missed something obvious.

We introduced @WithTestResource to change the value of @QuarkusTestResource.restrictToAnnotatedClass from false to true. The idea was that it was better to restrict test resources to the tests that are using them.

All was fine really, it looked like a good enhancement.

We started having people migrating to @WithTestResource, without using quarkus update (which actually take care of keeping the behavior preexisting in your app) and just changing @QuarkusTestResource to @WithTestResource (but adding it on all the tests given the new behavior) and then we started getting complaints of OOM and tests being slow.

To be fair:

- we poorly documented the consequences of switching to the new default behavior of the annotation

- you would expect new defaults to be better than the old ones

- tests are actually slow and leading to OOM when you are heavily using @WithTestResource

I for one completely missed the full consequences of this change.

For each test class annotated with @WithTestResource (if you keep the new default behavior):

- The local resources pointed to by @WithTestResource will be restarted - that's what we expect

- We will also have to restart Quarkus to take into account the new test resources (and potentially the new config contributed by these test resources) - that's where it starts to become slow

- Restarting Quarkus and getting rid of the test context means... also restarting the global test resources... Dev Services included. I.e. for each test, you will have to restart the containers. Which is slow... and additional bonus, Testcontainers start some background threads that are kept around forever and are leaking our class loaders (see https://github.com/quarkusio/quarkus/pull/42560 for a very bad experiment at fixing it. Spoiler: it doesn't work...) and obviously these threads are per test class.

So, if you use the default behavior of @WithTestResource, you end up with slow tests that are leaking memory (we fixed some of the leaks and reduced the size of the CL kept around but we are still leaking and if you have enough tests, you will eventually have an OOM).

We started adding some warnings and disclaimers to our doc and Javadoc but at the very end, I'm pretty sure we will end up with something like "don't use the new defaults except if you know what you're doing". Which kinda makes the point of having new defaults moot.

Now I don't blame anyone, I for one completely missed all this until we started having the reports and having some fun and memorable heap dumps explorations and experiments.

I think my recommendation would be to undeprecate @QuarkusTestResource for now until we have a plan - but I'm not entirely sure the plan will be easy to put in place. I'm also not sure it's viable: even if we fix the global test resources/Dev Services issue - which I don't expect to be easy to solve, we still have to restart Quarkus anyway, which won't fly if you have a lot of tests with local test resources.

Thoughts?

--

Guillaume

[Georgios Andrianakis]

I more than anyone carry the blame of not foreseeing how large of an effect this would have.

As for the path forward, I'm actually really undecided because on one hand the deprecation clearly leads people to simply swap the old annotation with the new one without looking at the value of restrictToAnnotatedClass, but on the other hand undeprecating it will likely mean we will be stuck with the old behavior forever (behavior which is problematic in a totally different sense - mainly a developer experience sense).

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-mBG3cddtko6pfucLYKwhDhQXJQTjs%3DGfyB9PAR8OR6A%40mail.gmail.com.

--

Georgios Andrianakis

Independent Contractor

[Guillaume Smet]

On Mon, Aug 26, 2024 at 11:05 AM Georgios Andrianakis <gand...@redhat.com> wrote:

I more than anyone carry the blame of not foreseeing how large of an effect this would have.

As for the path forward, I'm actually really undecided because on one hand the deprecation clearly leads people to simply swap the old annotation with the new one without looking at the value of restrictToAnnotatedClass, but on the other hand undeprecating it will likely mean we will be stuck with the old behavior forever (behavior which is problematic in a totally different sense - mainly a developer experience sense).

Well, I'm not sure the new behavior is better from a devex POV: sure your tests are better isolated (if that's what you want) but you end up with slow tests and memory leaks. Not sure how practical it is in reality and given we had a few reports in the middle of the summer, it doesn't look very good.

Now I'm not saying I want to undeprecate it forever. I just would like to avoid a massive migration to something that doesn't work very well.

That being said, I also think we should try to see if the situation is fixable and if there's a path forward for @WithTestResource. Because atm you can use it with the defaults only if you have very few tests with this behavior.

--

Guillaume

[Georgios Andrianakis]

It's probably dependent on the situation, but yeah, it can easily happen.

I guess I'm also worried that we if we revet the  deprecation, we'll end up not feeling enough pressure to fix it as best we can thus kicking the can down the road 

--

Guillaume

--
You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-%3DMiGR%3DLi80V-sYCoNzziz3NoR513uKa4GWBGpOBhDug%40mail.gmail.com.

[Martin Kouba]

On 26. 08. 24 9:04, Guillaume Smet wrote:
> Hi,
>
> I have been pondering sending this email for a week now as I gave a lot
> of thoughts to our journey fixing various issues causing OOM (see
> https://github.com/quarkusio/quarkus/issues/42471

> <https://github.com/quarkusio/quarkus/issues/42471>).

Hm, it's not a shame to admit that we made a mistake and revert the
change. Now, since we already released this API we can only come up with
"bad" and "worse" plans.

If the only difference between QuarkusTestResource and WithTestResource
is "restrictToAnnotatedClass() default true" then it might make more
sense to deprecate the WithTestResource#restrictToAnnotatedClass()
member, and introduce a new annotation member, something like
isolationStrategy() with enum values AUTO, RESTRICTED
(restrictToAnnotatedClass()==true) and SHARED
(restrictToAnnotatedClass()==false), where the AUTO is the default and
it means RESTRICTED unless the "quarkus.test.resource.isolation" system
property is used. This would allow users to mitigate the problem with
OOM and slow tests until we have a fix and still use the new annotation.

>
> Thoughts?

>
> --
> Guillaume
>
> --
> You received this message because you are subscribed to the Google
> Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to quarkus-dev...@googlegroups.com

> <mailto:quarkus-dev...@googlegroups.com>.

> To view this discussion on the web visit

> https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-mBG3cddtko6pfucLYKwhDhQXJQTjs%3DGfyB9PAR8OR6A%40mail.gmail.com <https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo-mBG3cddtko6pfucLYKwhDhQXJQTjs%3DGfyB9PAR8OR6A%40mail.gmail.com?utm_medium=email&utm_source=footer>.

--
Martin Kouba
Principal Software Engineer
Red Hat, Czech Republic

[Stephane Epardaud]

My personal experience with @QuarkusTestResource was that :

- It was very unintuitive that it applied to other classes, 

- In most cases people put it on a random number of test classes, sometimes with different configuration, which just made it weird,

- When running a single unannotated/unrelated test class, unrelated test resources would start, causing slow tests

IMO, @WithTestResource makes a lot more sense and is more intuitive, which is why we ended up with it.

That being said, it makes a lot of sense to be able to group test resources, and apply them to several test classes, but IMO the proper unit to apply that annotation may be the package? This would scope the test resource to all test classes to the package and subpackages? This would allow running a great number of tests with the same Quarkus instance (as was the case with `restrictToAnnotatedClass = false`) but in a way where scoping is quite obvious.

If we wanted to be more fine-grained than package+subpackages (which frankly I think is enough), we could make it a meta-annotation:

@WithTestResource(expensiveTestResource.class)

public @annotation WithExpensive {}

And then every test annotated with @WithExpensive gets grouped together in a single run (I suppose we could start by forbidding more than one such annotation on a given test class, to avoid having to deal with ven diagrams of groups, though we could have more than one test resource per meta annotation).

This should go a long way towards being able to properly scope which test resources apply to which tests, and how many Quarkus deployments we need for these groups.

Now, I agree, this is not really the heart of the discussion, which is about undeprecating @QuarkusTestResource, but perhaps the upgrade tool would move every instance of @QuarkusTestResource onto the package, instead of migrating them to @WithTestResource, and we'd keep the previous behaviour (single deployment, test resources shared, fast tests) while translating into a sane scoping that people could replace with @WithTestResource if they had the need?

[Martin Kouba]

On 26. 08. 24 15:57, Stephane Epardaud wrote:
> My personal experience with @QuarkusTestResource was that :
>
> - It was very unintuitive that it applied to other classes,
> - In most cases people put it on a random number of test classes,
> sometimes with different configuration, which just made it weird,
> - When running a single unannotated/unrelated test class, unrelated test
> resources would start, causing slow tests
>
> IMO, @WithTestResource makes a lot more sense and is more intuitive,
> which is why we ended up with it.
>
> That being said, it makes a lot of sense to be able to group test
> resources, and apply them to several test classes, but IMO the proper
> unit to apply that annotation may be the package? This would scope the
> test resource to all test classes to the package and subpackages? This
> would allow running a great number of tests with the same Quarkus
> instance (as was the case with `restrictToAnnotatedClass = false`) but
> in a way where scoping is quite obvious.

I like the idea of annotating a package.

>
> If we wanted to be more fine-grained than package+subpackages (which
> frankly I think is enough), we could make it a meta-annotation:
>
> @WithTestResource(expensiveTestResource.class)
> public @annotation WithExpensive {}

This could be also helpful. Esp. if you need to copy the annotation with
more complex config on many classes.

>
> And then every test annotated with @WithExpensive gets grouped together
> in a single run (I suppose we could start by forbidding more than one
> such annotation on a given test class, to avoid having to deal with ven
> diagrams of groups, though we could have more than one test resource per
> meta annotation).
>
> This should go a long way towards being able to properly scope which
> test resources apply to which tests, and how many Quarkus deployments we
> need for these groups.
>
> Now, I agree, this is not really the heart of the discussion, which is
> about undeprecating @QuarkusTestResource, but perhaps the upgrade tool
> would move every instance of @QuarkusTestResource onto the package,
> instead of migrating them to @WithTestResource, and we'd keep the
> previous behaviour (single deployment, test resources shared, fast
> tests) while translating into a sane scoping that people could replace
> with @WithTestResource if they had the need?
>

> --
> You received this message because you are subscribed to the Google
> Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to quarkus-dev...@googlegroups.com
> <mailto:quarkus-dev...@googlegroups.com>.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9uHRWW_EVNG7RyX%2BiJM0rvk6eiU18OFbDbTvdE%3DMqcHhA%40mail.gmail.com <https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9uHRWW_EVNG7RyX%2BiJM0rvk6eiU18OFbDbTvdE%3DMqcHhA%40mail.gmail.com?utm_medium=email&utm_source=footer>.

[Guillaume Smet]

On Mon, Aug 26, 2024 at 3:58 PM Stephane Epardaud <stephane...@gmail.com> wrote:

My personal experience with @QuarkusTestResource was that :

- It was very unintuitive that it applied to other classes, 

- In most cases people put it on a random number of test classes, sometimes with different configuration, which just made it weird,

- When running a single unannotated/unrelated test class, unrelated test resources would start, causing slow tests

IMO, @WithTestResource makes a lot more sense and is more intuitive, which is why we ended up with it.

It makes sense but it doesn't work at all in the current state of things.

If you have a bunch of tests and each requires a resource, you end up with a very slow and memory hungry test suite.

And that's what people manually updating are migrating to from the feedback we had (well we are saying isolated tests with specific resources are better after all).

Also keep in mind what I wrote above: we are restarting the restricted resources but also the global resources and Dev Services.

So sure it's better on paper but it's not manageable in the current state.

 

That being said, it makes a lot of sense to be able to group test resources, and apply them to several test classes, but IMO the proper unit to apply that annotation may be the package? This would scope the test resource to all test classes to the package and subpackages? This would allow running a great number of tests with the same Quarkus instance (as was the case with `restrictToAnnotatedClass = false`) but in a way where scoping is quite obvious.

If we wanted to be more fine-grained than package+subpackages (which frankly I think is enough), we could make it a meta-annotation:

@WithTestResource(expensiveTestResource.class)

public @annotation WithExpensive {}

And then every test annotated with @WithExpensive gets grouped together in a single run (I suppose we could start by forbidding more than one such annotation on a given test class, to avoid having to deal with ven diagrams of groups, though we could have more than one test resource per meta annotation).

This should go a long way towards being able to properly scope which test resources apply to which tests, and how many Quarkus deployments we need for these groups.

Now, I agree, this is not really the heart of the discussion, which is about undeprecating @QuarkusTestResource, but perhaps the upgrade tool would move every instance of @QuarkusTestResource onto the package, instead of migrating them to @WithTestResource, and we'd keep the previous behaviour (single deployment, test resources shared, fast tests) while translating into a sane scoping that people could replace with @WithTestResource if they had the need?

It's not that far from the discussion as:

- we need to decide if the new approach is viable and usable - and tweaks to make it so are definitely in the scope of this discussion

- the upgrade tool is actually clever enough to not change the behavior of existing code base (the recipe adjusts restrictToAnnotatedClass to keep the existing behavior) but... some people are migrating manually and new users will use whatever we say is best in our doc - and currently what we recommend doesn't really fly in a real codebase

That being said, we need to decide two things:

- what we do for 3.14/3.15 - meaning no crazy code changes

- what we do for the future - and yes, maybe package-scoped resources make sense

Anyway, I think we need to decide how we mitigate the issue and then we can discuss what we want for the future.

--

Guillaume

[Emmanuel Bernard]

Off Stephane proposal and probably not what we want but hey.

Package scope is hit and miss in Java, it makes lots of sense for some teams that package in one way, and does not make any sense for others (some put classes on a given package for visibility of members). Also it's not very intuitive, you have to read about it to understand it.

What about a scope with a default value, the default value being AllTests.class

@WithTestResource(scope=Expensive.class, value=DatabaseResource.class)

public class TestMeTest { 

...

}

And to implement restrictToAnnotatedClass=true, you can do

@WithTestResource(scope=TestMeTest.class, value=DatabaseResource.class)

public class TestMeTest { 

...

}

Now, this reverts the global behavior, so maybe the default scole should be AUTO with a defined behavior, but all restricted seems too excessive from the behavior being experienced here.

What I like with scope is that

1. it's optional

2. it gradually scales

But reading test profiles, it was trying to address that problem, is it too heavy to use? (user experience wise?)

BTW reading the JavaDocs I'm not 100% clear what's happening with restrictToAnnotatedClass=true. It is grouping tests with the same QuarkusTestResourceLifecycleManager list together to run in the same quarkus launch? And then rince and repeat for each unique groups?

Emmanuel

On Mon, Aug 26, 2024 at 3:58 PM Stephane Epardaud <stephane...@gmail.com> wrote:

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.

To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAKU9E9uHRWW_EVNG7RyX%2BiJM0rvk6eiU18OFbDbTvdE%3DMqcHhA%40mail.gmail.com.

[Eric Deandrea]

Is that entirely true, though? In the superheroes we are using @WithTestResource all over the place with its defaults in place and haven't seen any issues or OOMs. 

--

Guillaume

[Eric Deandrea]

On Monday, August 26, 2024 at 9:58:08 AM UTC-4 Stephane Epardaud wrote:

My personal experience with @QuarkusTestResource was that :

- It was very unintuitive that it applied to other classes, 

- In most cases people put it on a random number of test classes, sometimes with different configuration, which just made it weird,

- When running a single unannotated/unrelated test class, unrelated test resources would start, causing slow tests

IMO, @WithTestResource makes a lot more sense and is more intuitive, which is why we ended up with it.

That being said, it makes a lot of sense to be able to group test resources, and apply them to several test classes, but IMO the proper unit to apply that annotation may be the package? This would scope the test resource to all test classes to the package and subpackages? This would allow running a great number of tests with the same Quarkus instance (as was the case with `restrictToAnnotatedClass = false`) but in a way where scoping is quite obvious.

I'm not sure I would like the package idea. Different people have different ways of "slicing" their package structures. Generally your tests are in the same package as the class under test, so for instance applying the same resources to all tests in say the "rest" package where you are testing your jaxrs endpoints may not make sense.

[Eric Deandrea]

On Monday, August 26, 2024 at 10:41:15 AM UTC-4 Guillaume Smet wrote:

On Mon, Aug 26, 2024 at 3:58 PM Stephane Epardaud <stephane...@gmail.com> wrote:

My personal experience with @QuarkusTestResource was that :

- It was very unintuitive that it applied to other classes, 

- In most cases people put it on a random number of test classes, sometimes with different configuration, which just made it weird,

- When running a single unannotated/unrelated test class, unrelated test resources would start, causing slow tests

IMO, @WithTestResource makes a lot more sense and is more intuitive, which is why we ended up with it.

It makes sense but it doesn't work at all in the current state of things.

If you have a bunch of tests and each requires a resource, you end up with a very slow and memory hungry test suite.

And that's what people manually updating are migrating to from the feedback we had (well we are saying isolated tests with specific resources are better after all).

Isn't there a way to group all tests that have that annotation with the same test resource class (i.e. @WithTestResource(SomeClass.class)) and then keep the resources around for those tests, almost like an ordering. Then after those tests execute, move onto the next grouping of tests that use another resource?

[Guillaume Smet]

Hi,

After some discussion, we decided that it was better to revert this change for now in 3.14, until we have a better grasp of how we can fix the issues.

The patch has not been fully reverted meaning @WithTestResource is still around and works fine as to not break users who migrated to it. But the doc changes/migration guides/update recipes have all been reverted to not push users towards the new annotation until we figure out how to make it better. And @QuarkusTestResource has been undeprecated.

This work has been pushed to 3.14.1 that will be the first 3.14 officially released.

Note that we are perfectly aware that this is not entirely ideal as some people might have already migrated to the new annotation when they updated to 3.13 but we needed to take action to avoid the issue to spread (and new users to potentially have a bad image of Quarkus if their tests are too slow to begin with).

That's a step back but we really need to improve the behavior of this annotation before pushing it to prime time. Hopefully soon.

Thanks.

--

Guillaume

[Georgios Andrianakis]

Thanks a lot for doing the revert work and posting this!

I hope to have an improvement to discuss within the next few days

--

You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.

To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CALt0%2Bo9j-ncOzizUW7Hr-5E6MLVVew6RC0X2Y%3Dy2NyswjNzSgw%40mail.gmail.com.

[Georgios Andrianakis]

I have opened https://github.com/quarkusio/quarkus/pull/42852 as a proposal of what I had in mind.