Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TransactionalUniAsserter never fails with Hibernate Reactive #36599

Merged
merged 4 commits into from
Nov 2, 2023
Merged

TransactionalUniAsserter never fails with Hibernate Reactive #36599

merged 4 commits into from
Nov 2, 2023

Conversation

DavideD
Copy link
Contributor

@DavideD DavideD commented Oct 20, 2023 •
edited
Loading

Fix #36582

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 20, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should count at least 2 words to describe the change properly

This message is automatically generated by a bot.

@DavideD DavideD force-pushed the UniAsserter branch 2 times, most recently from a74217f to 88d638b Compare October 31, 2023 10:20
@DavideD DavideD changed the title UniAsserter TransactionalUniAsserter never fails with Hibernate Reactive Oct 31, 2023
@DavideD DavideD marked this pull request as ready for review October 31, 2023 10:22
@DavideD DavideD mentioned this pull request Oct 31, 2023
Closed
@DavideD DavideD requested a review from mkouba October 31, 2023 10:23
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor

mkouba commented Oct 31, 2023

Ugh, so an injected TransactionalUniAsserter parameter actually never worked because we never subscribed to it 🤦. Nice catch! Thanks.

@mkouba
Copy link
Contributor

mkouba commented Oct 31, 2023

@DavideD pls sort the imports ;-)

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/config area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/gradle Gradle area/hibernate-validator Hibernate Validator area/hibernate-orm Hibernate ORM area/kafka labels Oct 31, 2023
@DavideD DavideD force-pushed the UniAsserter branch 2 times, most recently from 61a086c to f4429cc Compare October 31, 2023 15:30
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 31, 2023
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@DavideD DavideD force-pushed the UniAsserter branch 3 times, most recently from 9b70d5f to 9cbaa30 Compare October 31, 2023 18:13
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@DavideD
Copy link
Contributor Author

DavideD commented Nov 1, 2023

Uff, there are some issues with the build. I will have another look on Thursday

mkouba
mkouba reviewed Nov 1, 2023
View reviewed changes
...ctive-panache/src/test/java/io/quarkus/it/panache/reactive/TransactionalUniAsserterTest.java Outdated
@Test
@RunOnVertxContext
public void testFailure(final TransactionalUniAsserter asserter) {
assertThrows(AssertionFailedError.class, asserter::fail);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, asserter::fail does not result in an exception. You would need to subscribe to the underlying Uni.

Unfortunately, JUnit5 does support the Junit4-style @Test(expected = NullPointerException.class). So we can't easily assert the failure...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Thanks

@DavideD
[quarkusio#36582] Fix bug: TransactionalUniAsserter never fails
5ddc8e5 
TransactionalUniAsserterTestMethodInvoker is a subclass of
RunOnVertxContextTestMethodInvoker.

The problem is that there were two separate pointers keeping
track of the asserter in the superclass and in the subclass.
This lead to only one pointer being initialized and, if the wrong
pointer was null-checked, the asserter was ignored causing the test
to never fail.

This commit fixes the issue by keeping only one reference to the
asserter in the superclass.
@DavideD
[quarkusio#36582] Minor clean up in TransactionalUniAsserterTest
b44a56c 
Use method reference where possible
@DavideD
[quarkusio#36582] Minor clean up around lambdas
a29061e
@DavideD
[quarkusio#36582] Add UnwrappableUniAsserter interface
c44eab7 
The goal is to extract the `asUni()` method from `UniAsserter`
@DavideD
Copy link
Contributor Author

DavideD commented Nov 2, 2023

I've removed the test for now. I need to focus on another issue. But, we need to add a way to test when something fails

@mkouba
Copy link
Contributor

mkouba commented Nov 2, 2023

I've removed the test for now. I need to focus on another issue. But, we need to add a way to test when something fails

It's a pity that we don't have a test that the injected TransactionalUniAsserter was actually used. I'll try to send a follow-up PR.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 2, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@mkouba mkouba merged commit eb344f0 into quarkusio:main Nov 2, 2023
22 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 2, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 2, 2023
@mkouba mkouba mentioned this pull request Nov 2, 2023
Merged
@gsmet gsmet modified the milestones: 3.6 - main, 3.5.1 Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/panache area/testing kind/bugfix
Projects
Quarkus Documentation
Status: Done
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.

Unit test always succeeds in combination with hibernate-reactive & panache
3 participants
@DavideD @mkouba @gsmet