-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Integration tests for hibernate-reactive-oracle #46132
Integration tests for hibernate-reactive-oracle #46132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looking great, I just have a few comments. The main one being about CI and native tests.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to make the build pass on Windows -- which doesn't have containers and thus cannot handle DB tests.
Essentially these tests need to be disabled by default, like in every other module, and enabled when passing -Dtest-containers
to Maven.
This comment has been minimized.
This comment has been minimized.
I am sorry about the merge commits in the PR, but I am new to open source, so I am still learning :) |
No problem. You may want to learn about rebasing: https://docs.github.com/en/get-started/using-git/about-git-rebase If you rebase your branch on |
It is getting worse, so I converted the PR to draft :) |
Do you need me to do the rebase, or are you still trying? :) |
I am still trying, but without success. If you can do it for me, that would be great! :) |
On it |
2caa357
to
3dc8fc8
Compare
Alright, I force-pushed a rebase. I must admit you got yourself in quite a situation 😅 Better avoid merge commits from the start, next time :) You can instruct git to rebase by default when pulling, instead of merging: https://coderwall.com/p/tnoiug/rebase-by-default-when-doing-git-pull FWIW, here's what I did:
I used "s" for every commit to squash everything: Got a conflict:
Solved it manually in my IDE, then:
Got another conflict:
Solved it manually in my IDE, then:
Got another conflict:
Solved it manually in my IDE, then:
Then rebase ended successfully, so I force-pushed to rewrite the git history of this branch (don't do this anywhere, but for PRs with a messy history, it's reasonable):
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
# Reactive config | ||
quarkus.datasource.reactive=true | ||
quarkus.datasource.reactive.url=${oracle.reactive.url} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanPetkov23 I think this should have been updated to reactive-oracle.url
to match your other changes.
CI is failing because it wasn't.
Thanks for the help and the patience :) |
This comment has been minimized.
This comment has been minimized.
a3d051d
to
7b2a830
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to fix the DB setup, because it didn't work. Also rebased on main.
It works locally, let's see how it goes on CI.
This comment has been minimized.
This comment has been minimized.
It is working locally on my machine too. Why CI tests are failing? |
I believe something leaves the Oracle container running after the new hibernate-reactive-oracle integration tests run, leading to problems when running later integration tests. |
…r ITs E.g. on quarkus-reactive-oracle-client-deployment, quarkus-it-jpa-oracle. Because the current setup doesn't seem to work.
7b2a830
to
a6dadc5
Compare
Status for workflow
|
Looks like the failures were caused by problems on the We're good to go, I'll merge. Thanks again @IvanPetkov23! |
Added tests for Hibernate Reactive with Oracle. Fixes: #23820