Introduce the ability to run test methods on the vertx context #17143
Conversation
|
@cescoffier @FroMage @stuartwdouglas I opened this as a draft to give you an idea of what problem I am trying to solve. |
test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
Outdated
Show resolved
Hide resolved
test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
Outdated
Show resolved
Hide resolved
|
@stuartwdouglas @FroMage do you think this is worth pursuing or not? |
|
Is this something we are going to need more generally given the threading model of Hibernate Reactive? If so maybe it should be a CDI interceptor |
|
Yeah, that's what prompted me to start this. Users often inject the repository into tests in order to assert the state of the database, but currently things just fall over in that case. You are proposing that we introduce a CDI interceptor that essentially does the same thing? |
|
I mean you would apply the interceptor to the test, but it means you could use it in your application as well. If you have done an operation that leaves you off the IO thread this would allow you to dispatch back to it. Not really sure if this is useful or not though. |
|
Ah, I see what you mean. So implicitly switching threads when there is an issue like the one in question. Sounds interesting, but I don't know if being explicit or implicit is better in this case. Let's see what others think as well |
|
Huh, I'm interested in this, because I'm looking at The issue is that I don't understand why it runs from the CLI. |
|
I suspect something is not respecting the |
|
Indeed, surefire runs the tests like this: Even though I explicitely enabled assertions. |
|
@FroMage how do you pass JVM args to surefire? |
|
For sure fire, assertions are enabled from the configuration:
|
|
Should I mode this PR ahead? What do you folks think?
…On Sat, May 22, 2021, 07:49 Clement Escoffier ***@***.***> wrote:
For sure fire, assertions are enabled from the configuration:
true
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#17143 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBMDP4RXDOJUAH23ZIILT3TO4ZWRANCNFSM44VVGXDA>
.
|
|
I like this idea a lot. But I wonder if it's enough. The Vert.x JUnit extension has an extra injected param available to make it easy to test async stuff: https://vertx.io/docs/vertx-junit5/java/ ATM we mostly call |
|
So do you think we should allow returning a Uni from the test and wait and assert no error occurred ourselves? |
|
I'm also very interested in this, not only because we have tests failure from IntelliJ with |
|
So should I clean this one up an take it out of draft? |
|
@gwenneg do you have an example I can use to test? |
I just now see what you mean (after actually trying an example). So yeah, we will need some kind of way to make it easy for users to write assertions without blocking |
|
I am thinking that perhaps we allow passing some kind of asserter object as a method param. A sample test would then be written something like this: @QuarkusTest
public class SessionTest {
@Inject
Mutiny.Session session;
@RunOnVertxContext
@ActivateRequestContext // should we enable this by default?
@Test
public void test(UniAsserter asserter) {
asserter.assertItem(allFruits(), f -> { // the API of the asserter obviously needs to be worked out
assertFalse(f.isEmpty());
})
}
private Uni<List<Fruit>> allFruits() {
return session.createNamedQuery("Fruits.findAll", Fruit.class).getResultList();
}
}What do you folks think? |
|
With the latest commit I added one can write: @QuarkusTest
public class SessionTest {
@Inject
Mutiny.SessionFactory sessionFactory;
@RunOnVertxContext
@Test
public void test1(UniResult<List<Fruit>> asserter) {
asserter.assertItem(allFruits(), fruits -> assertThat(fruits).hasSize(3));
}
private Uni<List<Fruit>> allFruits() {
return sessionFactory.openSession().createNamedQuery("Fruits.findAll", Fruit.class).getResultList();
}
}The implementation is far from pretty (due to the CL and Thread juggling), but the end result should be pretty simple for users. WDYT? |
|
That looks nice. Why do we need to specify the type of thing being asserted? What if we have more than one assertion on different things? Lemme give you a real-world example from HR/Panache IT: Assertions.assertEquals(0, mockablePersonRepository.count().await().indefinitely());
Mockito.when(mockablePersonRepository.count()).thenReturn(Uni.createFrom().item(23l));
Assertions.assertEquals(23, mockablePersonRepository.count().await().indefinitely());
Mockito.when(mockablePersonRepository.count()).thenReturn(Uni.createFrom().item(42l));
Assertions.assertEquals(42, mockablePersonRepository.count().await().indefinitely());
Mockito.when(mockablePersonRepository.count()).thenCallRealMethod();
Assertions.assertEquals(0, mockablePersonRepository.count().await().indefinitely());
Mockito.verify(mockablePersonRepository, Mockito.times(4)).count();Which uses the And: createBug7102()
.flatMap(person -> {
return getBug7102(person.id)
.flatMap(person1 -> {
Assertions.assertEquals("pero", person1.name);
return updateBug7102(person.id);
})
.flatMap(v -> getBug7102(person.id))
.map(person2 -> {
Assertions.assertEquals("jozo", person2.name);
return null;
});
}).flatMap(v -> Person.deleteAll())
.await().indefinitely();Which chains everything, for one last |
|
Good point! We definitely don't need to use the type, I just added for simplicity. So if we agree on the approach, what do you think the API of the asserter should look like? |
|
lemme try some pseudo-code and get back to you |
|
With: interface Asserter {
<T> void assertThat(Supplier<Uni<T>> uni, Consumer<T> asserter);
<T> void execute(Supplier<Uni<T>> uni);
<T> void execute(Runnable c);
}I could rewrite the first one with: ass.assertThat(() -> mockablePersonRepository.count(), count -> Assertions.assertEquals(0, count));
ass.execute(() -> Mockito.when(mockablePersonRepository.count()).thenReturn(Uni.createFrom().item(23l)));
ass.assertThat(() -> mockablePersonRepository.count(), count -> Assertions.assertEquals(23, count));
ass.execute(() -> Mockito.when(mockablePersonRepository.count()).thenReturn(Uni.createFrom().item(42l)));
ass.assertThat(() -> mockablePersonRepository.count(), count -> Assertions.assertEquals(42, count));
ass.execute(() -> Mockito.when(mockablePersonRepository.count()).thenCallRealMethod());
ass.assertThat(() -> mockablePersonRepository.count(), count -> Assertions.assertEquals(0, count));
ass.execute(() -> Mockito.verify(mockablePersonRepository, Mockito.times(4)).count());
Person p = new Person();
ass.execute(() -> Mockito.when(mockablePersonRepository.findById(12l)).thenReturn(Uni.createFrom().item(p)));
ass.assertThat(() -> mockablePersonRepository.findById(12l), p2 -> Assertions.assertSame(p, p2));
ass.assertThat(() -> mockablePersonRepository.findById(42l), p2 -> Assertions.assertNull(p2));
ass.execute(() -> mockablePersonRepository.persist(p));
ass.execute(() -> {
Assertions.assertNull(p.id);
Mockito.when(mockablePersonRepository.findById(12l)).thenThrow(new WebApplicationException());
try {
mockablePersonRepository.findById(12l);
Assertions.fail();
} catch (WebApplicationException x) {
}
Mockito.when(mockablePersonRepository.findOrdered()).thenReturn(Uni.createFrom().item(Collections.emptyList()));
});
ass.assertThat(() -> mockablePersonRepository.findOrdered(), v -> Assertions.assertTrue(v.isEmpty()));
ass.execute(() -> {
Mockito.verify(mockablePersonRepository).findOrdered();
Mockito.verify(mockablePersonRepository, Mockito.atLeastOnce()).findById(Mockito.any());
Mockito.verify(mockablePersonRepository).persist(Mockito.<Person> any());
Mockito.verifyNoMoreInteractions(mockablePersonRepository);
});Doesn't really impress me. Relies on calls to The second looks better: ass.assertThat(() -> createBug7102(), person -> {
ass.assertThat(() -> getBug7102(person.id), person1 -> Assertions.assertEquals("pero", person1.name));
ass.assertThat(() -> updateBug7102(person.id), person2 -> Assertions.assertEquals("jozo", person2.name));
ass.execute(() -> Person.deleteAll());
}); |
|
Thanks for the great examples! I'll have a look tomorrow |
|
And I noticed that due to this exception: java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-31
at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:30)
at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:61)
at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:42)
at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102(PanacheFunctionalityTest.java:185)
at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102InOneTransaction(PanacheFunctionalityTest.java:166)
…Which essentially means that in this code that looks like a normal test code: @DisabledOnNativeImage
@ReactiveTransactional
@Test
void testTransaction() {
Transaction transaction = Panache.currentTransaction().await().indefinitely();
Assertions.assertNotNull(transaction);
}I can't actually call So, not sure how to proceed. I guess this would work with |
|
Seems junit-team/junit5#2304 confirms non-void methods are just ignored by JUnit ATM. |
| @Override | ||
| public void handle(Void event) { | ||
| ManagedContext requestContext = Arc.container().requestContext(); | ||
| if (requestContext.isActive()) { |
There was a problem hiding this comment.
Isn't this fishy? AFAIK the request context is always active on the test methods, no? If so, it should just be propagated, no?
There was a problem hiding this comment.
It's actually needed. Without it I am getting ContextNotActiveException in the Panache tests
|
Right, my PR has the same.
That is why I added the Asserter in the first place
…On Wed, Jun 9, 2021, 17:33 Stéphane Épardaud ***@***.***> wrote:
And I noticed that due to this exception:
java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-31
at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:30)
at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:61)
at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:42)
at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102(PanacheFunctionalityTest.java:185)
at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102InOneTransaction(PanacheFunctionalityTest.java:166)
…
Which essentially means that in this code that looks like a normal test
code:
@DisabledOnNativeImage
@ReactiveTransactional
@test
void testTransaction() {
Transaction transaction = Panache.currentTransaction().await().indefinitely();
Assertions.assertNotNull(transaction);
}
I can't actually call await because I'm suddenly on the Vert.x event loop
thread and not allowed to block it. I also can't make the test method
return a Uni to make my test method properly async (the interceptor could
then know how to deal with that) because JUnit refuses to run non-void
methods for some reason (claims the test passed, but didn't invoke it).
So, not sure how to proceed. I guess this would work with Asserter, but
then we have to force users to use it when doing test methods on the Vert.x
event loop thread. I guess your PR must have the same issue, that tests
can't actually use await() when running on the Vert.x context, right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17143 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBMDPZXRNG27T3K7IV5I4TTR53UDANCNFSM44VVGXDA>
.
|
|
Right, this shouldn't stickly be necessary
…On Wed, Jun 9, 2021, 19:29 Georgios Andrianakis ***@***.***> wrote:
Right, my PR has the same.
That is why I added the Asserter in the first place
On Wed, Jun 9, 2021, 17:33 Stéphane Épardaud ***@***.***>
wrote:
> And I noticed that due to this exception:
>
> java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-31
>
> at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:30)
>
> at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:61)
>
> at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:42)
>
> at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102(PanacheFunctionalityTest.java:185)
>
> at io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testBug7102InOneTransaction(PanacheFunctionalityTest.java:166)
>
> …
>
> Which essentially means that in this code that looks like a normal test
> code:
>
> @DisabledOnNativeImage
>
> @ReactiveTransactional
>
> @test
>
> void testTransaction() {
>
> Transaction transaction = Panache.currentTransaction().await().indefinitely();
>
> Assertions.assertNotNull(transaction);
>
> }
>
> I can't actually call await because I'm suddenly on the Vert.x event
> loop thread and not allowed to block it. I also can't make the test method
> return a Uni to make my test method properly async (the interceptor
> could then know how to deal with that) because JUnit refuses to run non-
> void methods for some reason (claims the test passed, but didn't invoke
> it).
>
> So, not sure how to proceed. I guess this would work with Asserter, but
> then we have to force users to use it when doing test methods on the Vert.x
> event loop thread. I guess your PR must have the same issue, that tests
> can't actually use await() when running on the Vert.x context, right?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#17143 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABBMDPZXRNG27T3K7IV5I4TTR53UDANCNFSM44VVGXDA>
> .
>
|
|
Here is a crazy thought: Would it be possible to have an event loop thread that is reserved for tests and where blocking is allowed? |
|
I don't think this would work : the idea around the async drivers is that they have thread affinity with the IO thread that they sent their queries from. So they can't block. Otherwise we'd be able to even just use a Vert.x worker thread with affinity. |
|
Yeah, I talked with @cescoffier as well and he will also write why he thinks it will hard to do that |
|
There are two problems:
|
|
Well, IMO I can only use |
|
👍🏼 So what is missing from this to get you started? |
This leaves the implementations of TestMethodInvoker mostly free of having to ClassLoading issues
@FroMage ^ |
|
I should hopefully be ready to make my PR work with your PR by tomorrow. I had to fix a few things in your branch to get it to work, now I just have to verify that I have enough tests. |
|
That's awesome! Should I close this PR then and have everything incorporated in yours? |
@FroMage ^ |
|
Probably yes, I've added some fixes there for you to check. |
|
Hey @geoand, thanks for your work on |
|
Good point! I don't think I added something to emulate assertThrows... Can you open an issue for that? |
|
Sure. |
Relates to: #15131