Make Hibernate Reactive tests use @RunOnVertxContext and upgrade the Context validity checks #23905

geoand · 2022-02-23T11:18:20Z

[edited by @Sanne ]
This makes the internal integration tests of Hibernate Reactive use the @RunOnVertxContext annotation and also:

makes the @RunOnVertxContext also work for internal tests
introduces an attribute on @RunOnVertxContext to specify the kind of context
Changes/renames the quarkus-junit5-vertx - necessary to resolve cross-module dependencies
Upgrade the checks that Hibernate Reactive performs to validate it running on the right context: it will now fail by default if it's run on an un-blessed context.

This resolves all remaining concerns I had in regards to context usage for H.R.

N.B. This is a breaking change because the module name of quarkus-junit5-vertx was changed to quarkus-test-vertx (as it now needs to support quarkus-junit5-internal), the dependency on quarkus-junit5 was removed and the API changed packages (from io.quarkus.test.junit.vertx to io.quarkus.test.vertx).

cescoffier · 2022-02-23T11:21:42Z

Because you are creating a root context and not a duplicated context :-)

geoand · 2022-02-23T11:22:48Z

I need the magic sauce to get to that duplicated context 😉

geoand · 2022-02-23T11:24:32Z

Nevermind, found it, so let me try

geoand · 2022-02-23T11:26:39Z

Success!

geoand · 2022-02-23T11:30:38Z

@Sanne all this PR needs now if the Hibernate tests to be ported (which is what the last commit started). Do you want to do that?

Sanne · 2022-02-23T12:09:48Z

Many thanks! Nice, there are some similarities with what I was toying with yesterday:

https://github.com/quarkusio/quarkus/compare/main...Sanne:CheckContext?expand=1

In particular I was preparing to introduce an attribute for the @RunOnVertxContext to control if the context shuld be duplicated, to make it more explicit. WDYT should we add it?

Sanne · 2022-02-23T12:11:37Z

How important is the module rename commit cf06953?

If possible I'd refrain from that, so to not introduce breaking changes.

...ernate-reactive/deployment/src/test/java/io/quarkus/hibernate/reactive/SchemaUpdateTest.java

cescoffier · 2022-02-23T12:06:34Z

...ernate-reactive/deployment/src/test/java/io/quarkus/hibernate/reactive/SchemaUpdateTest.java

@@ -28,12 +27,15 @@
    @Inject
    Mutiny.SessionFactory sessionFactory;

+    @RunOnVertxContext


That's not enough, what kind of context is it running on?
Event loop context, worker context, duplicated context?

The default should be a duplicated context, but we may need parameters to create the right type of context.

Right - like I suggested I believe we should have some attributes on the annotation to make things explicit and controllable.

But I think "duplicated" could be a reasonable default.

https://github.com/quarkusio/quarkus/compare/main...Sanne:CheckContext?expand=1#diff-b4dcc8850c70f1ef3ed1c7c55fd3cd5b12db20e8b41f3cf8904e0d6ef3536333L77-R86

But I'm ok fixing that as a follow up myself, if there's agreement on the direction :)

Since you folks know more about what needs to be done, I'm fine with letting you add more metadata to the annotation :)

Cool, yes thanks a lot to implement the testing tool bits. I was trying yesterday but honestly got lost - not familiar with it at all so I appreciate you doing that part.

Yeah, it's too easy to get lost with all the necessary classloader hacks...

cescoffier · 2022-02-23T12:09:51Z

...-framework/vertx/src/main/java/io/quarkus/test/vertx/RunOnVertxContextTestMethodInvoker.java

@@ -74,7 +75,7 @@ public Object invoke(Object actualTestInstance, Method actualTestMethod, List<Ob
        CompletableFuture<Object> cf = new CompletableFuture<>();
        RunTestMethodOnContextHandler handler = new RunTestMethodOnContextHandler(actualTestInstance, actualTestMethod,
                actualTestMethodArgs, uniAsserter, cf);
-        vertx.getOrCreateContext().runOnContext(handler);
+        VertxContext.getOrCreateDuplicatedContext(vertx).runOnContext(handler);


I think we need to give more flexibility. Yes, the default should be a duplicated context with its root being the event loop context, but we may need some other cases.

That being said, that should always run on a duplicated context.

geoand · 2022-02-23T12:21:17Z

If possible I'd refrain from that, so to not introduce breaking changes.

Not really possible to avoid unfortunately because of the dependency of the existing module on quarkus-junit5.

geoand · 2022-02-23T12:23:37Z

In particular I was preparing to introduce an attribute for the @RunOnVertxContext to control if the context shuld be duplicated, to make it more explicit. WDYT should we add it?

Yeah, I think it makes sense. Feel free to make any changes you like in the PR :)

Sanne · 2022-02-24T14:04:15Z

I think this is done now - could @geoand and/or @cescoffier have another look?

geoand · 2022-02-24T14:06:13Z

LGTM

gsmet · 2022-02-24T14:17:06Z

@Sanne does that mean that anyone using HR will have to annotate their tests?

Sanne · 2022-02-24T14:44:39Z

It should only affect users of the "internal" testing extension

gsmet · 2022-02-24T14:45:31Z

It should only affect users of the "internal" testing extension

Ok, cool, good to know.

…safe

…ght execution context

geoand · 2022-02-25T09:36:09Z

Force pushed another fix

Sanne · 2022-02-25T10:32:29Z

@geoand for who is it actually a breaking change?

To be clear Hibernate Reactive always had the requirement to run on a vert.x context - it just so happens it was able to bypass this requirement in these specific integration tests because the use the internal testing tools which aren't meant for end users.

The only user-affecting change I see is that @RunOnVertxContext now executes (by default) on a duplicated context; but

I don't expect that to have negative impact on tests - it probably was the original intent
The obscure annotation wasn't adverstised
We give the option to run on the global context instead

Let me know if you think I'm forgetting some aspect :)

geoand · 2022-02-25T10:47:43Z

It breaks anyone using @RunOnVertxContext because:

The maven module that contains it changed name
The dependencies of the maven module that contained it changed
The package of the annotation changed

But as we both said, this is very unlikely to have been used by anybody as we never mentioned it anywhere.

So to summarize, it's breaking, but it's rather unlikely to affect anyone.

Sanne · 2022-02-25T10:59:52Z

@geoand ah, sure. But you clarified that in the description already - I thought you meant there was more breaking stuff on top of it.

I think we're good in that case - perhaps I wouldn't have changed the module names as I said earlier, but you said you had no choice :) If we're doing what we can IMO we'll need to go ahead and ask for forgiveness.

geoand · 2022-02-25T11:00:56Z

Agreed

edeandrea · 2022-03-03T13:42:57Z

But as we both said, this is very unlikely to have been used by anybody as we never mentioned it anywhere.

So to summarize, it's breaking, but it's rather unlikely to affect anyone.

That may not be 100% true, as it has broken the super heroes sample app. It doesn't break people just using the @RunOnVertxContext, but also breaks anyone using the UniAsserter, which would be pretty much anyone writing transactional-based tests with Hibernate Reactive/Panache reactive.

See #23612 (comment)

Just my opinion here (and I'm late to this as I am just returning from PTO - so what's done is done), but introducing breaking changes like this shouldn't be done in minor versions. I also understand that I don't fully understand the context which required the breaking change, so circumstances certainly could have warranted it, but it should be avoided in minor versions unless absolutely necessary.

* Bump quarkus.platform.version from 2.7.2.Final to 2.7.3.Final Bumps `quarkus.platform.version` from 2.7.2.Final to 2.7.3.Final. Updates `quarkus-bom` from 2.7.2.Final to 2.7.3.Final - [Release notes](https://github.com/quarkusio/quarkus-platform/releases) - [Commits](quarkusio/quarkus-platform@2.7.2.Final...2.7.3.Final) Updates `quarkus-maven-plugin` from 2.7.2.Final to 2.7.3.Final --- updated-dependencies: - dependency-name: io.quarkus.platform:quarkus-bom dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: io.quarkus.platform:quarkus-maven-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * Fix based on quarkusio/quarkus#23905 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Eric Deandrea <eric.deandrea@gmail.com>

Sanne · 2022-03-03T14:35:18Z

hi @edeandrea , could you point me to the test in the super heroes app that was affected?

It would help me to see a concrete example, as think this only affects either internal testing API or code which wasn't running in the right context to begin with. I might be wrong though :)

edeandrea · 2022-03-03T14:38:05Z

@Sanne https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-heroes/src/test/java/io/quarkus/sample/superheroes/hero/repository/HeroRepositoryTests.java (after I made changes).

I had to change the artifactId of the dependency in pom.xml as well and then update the java package of the UniAsserter class.

Here's the commit: quarkusio/quarkus-super-heroes@5050d31

cescoffier · 2022-03-03T15:43:45Z

so, the problem is the artifactId change. @geoand do you believe we can to a relocation?

edeandrea · 2022-03-03T15:45:19Z

so, the problem is the artifactId change. @geoand do you believe we can to a relocation?

Its not just the artifactId change - also the package relocation from io.quarkus.test.junit.vertx to io.quarkus.test.vertx

geoand · 2022-03-03T15:51:24Z

Yeah, I didn't do the relocation thing because the package also had to change.

Sanne · 2022-03-03T15:57:36Z

I guess I don't fully follow why the package had to change? Could we have a deprecated copy of the annotation in the previous location?

edeandrea · 2022-03-03T15:58:46Z

UniAsserter isn't an annotation - but it probably could have been deprecated and either extended in a new package (or moved to a new package and extended/deprecated in the old)?

geoand · 2022-03-03T16:05:03Z

The old package didn't make sense after the repurposing of the API to work for both internal and user facing tests

Sanne · 2022-03-03T19:31:44Z

The old package didn't make sense after the repurposing of the API to work for both internal and user facing tests

Ok but that's not a reason that it had to change ? I would suggest introducing a deprecated alternative for both the removed types.

geoand · 2022-03-04T05:30:46Z

There were too many changes that stacked up which is why I didn't see the reason for trying to keep compatibility

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/hibernate-reactive area/panache area/persistence area/testing labels Feb 23, 2022

geoand force-pushed the qut-vertx branch from 102a556 to b056580 Compare February 23, 2022 11:30

geoand added the release/breaking-change label Feb 23, 2022

cescoffier requested changes Feb 23, 2022

View reviewed changes

quarkus-bot bot added the area/vertx label Feb 23, 2022

Sanne force-pushed the qut-vertx branch from c2008ee to 6d2eb0a Compare February 24, 2022 13:29

Sanne marked this pull request as ready for review February 24, 2022 14:03

geoand changed the title ~~WIP - Make Hibernate Reactive tests use @RunOnVertxContext~~ Make Hibernate Reactive tests use @RunOnVertxContext Feb 24, 2022

geoand requested a review from cescoffier February 24, 2022 14:04

Sanne changed the title ~~Make Hibernate Reactive tests use @RunOnVertxContext~~ Make Hibernate Reactive tests use @RunOnVertxContext and upgrade the Context validity checks Feb 24, 2022

Sanne added the triage/backport? label Feb 24, 2022

Sanne approved these changes Feb 24, 2022

View reviewed changes

Sanne added 3 commits February 25, 2022 11:35

Have the duplicated contexts created by @RunOnVertxContext marked as …

5da0e87

…safe

Update the Hibernate Reactive tests to use @RunOnVertxContext

90265a1

Enable stricter checks for Hibernate Reactive to be running in the ri…

45bd41f

…ght execution context

geoand force-pushed the qut-vertx branch from 6b817d3 to 45bd41f Compare February 25, 2022 09:36

geoand merged commit 89b8b84 into quarkusio:main Feb 25, 2022

quarkus-bot bot added this to the 2.8 - main milestone Feb 25, 2022

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 25, 2022

geoand deleted the qut-vertx branch February 25, 2022 14:35

gsmet modified the milestones: 2.8 - main, 2.7.3.Final Feb 28, 2022

gsmet removed the triage/backport? label Feb 28, 2022

geoand mentioned this pull request Mar 1, 2022

[CI] - Quarkus Super Heroes + Quarkus main #23612

Closed

edeandrea added a commit to quarkusio/quarkus-super-heroes that referenced this pull request Mar 3, 2022

Fix based on quarkusio/quarkus#23905

25a763a

Make Hibernate Reactive tests use @RunOnVertxContext and upgrade the Context validity checks #23905

Make Hibernate Reactive tests use @RunOnVertxContext and upgrade the Context validity checks #23905

Conversation

geoand commented Feb 23, 2022 • edited by Sanne

cescoffier commented Feb 23, 2022

geoand commented Feb 23, 2022

geoand commented Feb 23, 2022

geoand commented Feb 23, 2022

geoand commented Feb 23, 2022

Sanne commented Feb 23, 2022

Sanne commented Feb 23, 2022 • edited

cescoffier Feb 23, 2022

Choose a reason for hiding this comment

Sanne Feb 23, 2022 • edited

Choose a reason for hiding this comment

geoand Feb 23, 2022

Choose a reason for hiding this comment

Sanne Feb 23, 2022

Choose a reason for hiding this comment

geoand Feb 23, 2022

Choose a reason for hiding this comment

cescoffier Feb 23, 2022

Choose a reason for hiding this comment

geoand commented Feb 23, 2022

geoand commented Feb 23, 2022

Sanne commented Feb 24, 2022

geoand commented Feb 24, 2022

gsmet commented Feb 24, 2022

Sanne commented Feb 24, 2022 via email • edited

gsmet commented Feb 24, 2022

geoand commented Feb 25, 2022

Sanne commented Feb 25, 2022

geoand commented Feb 25, 2022 • edited

Sanne commented Feb 25, 2022

geoand commented Feb 25, 2022

edeandrea commented Mar 3, 2022 • edited

Sanne commented Mar 3, 2022

edeandrea commented Mar 3, 2022

cescoffier commented Mar 3, 2022

edeandrea commented Mar 3, 2022 • edited

geoand commented Mar 3, 2022

Sanne commented Mar 3, 2022

edeandrea commented Mar 3, 2022

geoand commented Mar 3, 2022

Sanne commented Mar 3, 2022

geoand commented Mar 4, 2022

geoand commented Feb 23, 2022 •

edited by Sanne

Sanne commented Feb 23, 2022 •

edited

Sanne Feb 23, 2022 •

edited

Sanne commented Feb 24, 2022 via email •

edited

geoand commented Feb 25, 2022 •

edited

edeandrea commented Mar 3, 2022 •

edited

edeandrea commented Mar 3, 2022 •

edited