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

JPA test cleanup #37329

Merged
merged 4 commits into from
Dec 5, 2023
Merged

JPA test cleanup #37329

merged 4 commits into from
Dec 5, 2023

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Nov 27, 2023 •
edited
Loading

Opening this draft so I don't forget about it.

Based on #36978 , which must be merged first.

For now this includes only this:

  • Migrate all JPA tests to RestEasy Reactive (instead of using Servlets)

I had to do it anyway because I couldn't for the life of me debug test failures with this servlet stuff.

I wonder if I should also do the following:

  • Use the recommended approach to access the entity manager: inject it, instead of using createEntityManager
    That will probably require adding a dedicated test using JPA's createEntityManager directly, so that we have at least some coverage for that?
  • Use the recommended approach to start/end transactions: @Transactional or QuarkusTransaction, instead of entityManager.getTransaction()
    That will probably require adding a dedicated test using entityManager.getTransaction(), so that we have at least some coverage for that?
  • Move all JPA integration tests under a common directory?
  • Move all JPA + Hibernate ORM integration tests under a common directory?

WDYT @gsmet / @Sanne?

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/hibernate-orm Hibernate ORM area/hibernate-search Hibernate Search area/panache area/persistence OBSOLETE, DO NOT USE area/spring Issues relating to the Spring integration labels Nov 27, 2023
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 27, 2023

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm,hibernate-search)

@github-actions GitHub Actions
Copy link

🎊 PR Preview cd89658 has been successfully built and deployed to https://quarkus-pr-main-37329-preview.surge.sh/version/main/guides/

@gsmet
Copy link
Member

gsmet commented Nov 28, 2023

All of this makes sense. Just remember you also need to adjust the JSON file with native tests if you move the IT modules around.

@yrodiere yrodiere force-pushed the jpa-test-cleanup branch 2 times, most recently from 4be58fe to 861e286 Compare December 4, 2023 16:25
@yrodiere yrodiere marked this pull request as ready for review December 4, 2023 16:25
@yrodiere yrodiere requested a review from gsmet December 4, 2023 16:25
@yrodiere
Copy link
Member Author

yrodiere commented Dec 4, 2023

@gsmet Thanks. I did not move the modules in the end, because I need to move to some other task. But I think this is a good first step and ready to merge :)

About the review... Hum... good luck? More seriously, I'd recommend reviewing commit by commit, that way you can skip the larger diffs more easily.

gsmet
gsmet approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

LGTM!

@gsmet
Copy link
Member

gsmet commented Dec 4, 2023

There are a bunch of tests failing (at least on Windows):


Error:  io.quarkus.it.jpa.proxy.ProxyTest.testEnhancedProxies -- Time elapsed: 0.029 s <<< FAILURE!
java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.
Expected: is "OK"
  Actual: <html><body><h1>Resource not found</h1></body></html>

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@yrodiere yrodiere removed area/documentation area/panache area/spring Issues relating to the Spring integration area/hibernate-search Hibernate Search area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Dec 5, 2023
@yrodiere
Migrate all JPA tests to RestEasy Reactive
18d1034 
Because:

1. We recommend that people use RestEasy Reactive, so we might as well
   take our own advice.
2. I've had enough with copy/pasted, manual exception handling
   ("reportException(...)"), especially since it's buggy (it doesn't
   take into account the fact that exceptions may have a null message,
   e.g. NPEs);
3. I don't want to deal with servlets on a daily basis, thank you very
   much.
@yrodiere
Add test specifically for using Hibernate ORM in a non-quarkus way
1e4b135
@yrodiere
Move tests that are not specific to H2 out of integration-tests/jpa-h2
1d558b7
@yrodiere
Use a more Quarkus-native style for JPA tests
40e209b 
1. Inject the entity manager and make use of the transaction-scoped
   entity manager proxy.
2. Use QuarkusTransaction instead of em.getTransaction() (which isn't
   even supposed to work in JTA environments...)
@yrodiere
Copy link
Member Author

yrodiere commented Dec 5, 2023

Right, I forgot to run this particular module locally... Note I did run all the others, downloading all the DB container images just for this 😎

Anyway, I fixed the problems: should be good now.

@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 5, 2023
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Dec 5, 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.

@yrodiere yrodiere merged commit 45a7c9d into quarkusio:main Dec 5, 2023
20 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 5, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 5, 2023
@yrodiere yrodiere deleted the jpa-test-cleanup branch January 29, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE
Projects
None yet
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.
2 participants
@yrodiere @gsmet