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.

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

Fix swallowed failures in Reactive SQL Client tests #38106

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Jan 9, 2024

I have no idea why these tests swallow failures, but it doesn't seem necessary to test the CDI bean producer...
I mean we're not even checking that failures occur, so I really don't know why we would trigger those failures.

I'd like to fix this in order to reuse application-default-datasource.properties in other tests for another PR. Currently it's not possible, since that file is incomplete: depending on the client, sometimes it's completely empty, sometimes it's lacking credentials, ...

Hey @tsegismont can you confirm I didn't overlook some important reason to have connections fail and ignore the failures here?

This comment has been minimized.

@yrodiere
Copy link
Member Author

yrodiere commented Jan 9, 2024

Well the tests pass once we fix the configuration and stop ignoring failures, so I guess this is good to merge?

@gsmet
Copy link
Member

gsmet commented Jan 9, 2024

I might be mistaken but I wonder if it was to have the tests working even when you don't have a Docker environment.

@tsegismont
Copy link
Contributor

@yrodiere @gsmet To be honest, I can't remember why the failures are ignored.

Looking at the history, I think initially it was convenient to have the tests passing even when no database was available. But, later on, the tests have been disabled by default and can be enabled by profile.

So I believe it's fine to stop ignoring failures now.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @yrodiere

@yrodiere
Copy link
Member Author

Np, thanks for confirming @tsegismont !

@gsmet would you mind approving? Apparently @tsegismont's approval is not enough :)

@yrodiere
Copy link
Member Author

@gsmet would you mind approving? Apparently @tsegismont's approval is not enough :)

Bump :)

@yrodiere yrodiere force-pushed the fix-swallowed-sql-client-failures branch from 3d6c37f to 7a3b734 Compare January 15, 2024 11:34
Copy link

quarkus-bot bot commented Jan 15, 2024

✔️ 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.

You can consult the Develocity build scans.

@gastaldi gastaldi merged commit f547ee5 into quarkusio:main Jan 15, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Jan 15, 2024
@yrodiere
Copy link
Member Author

Thanks @gastaldi!

@yrodiere yrodiere deleted the fix-swallowed-sql-client-failures branch January 29, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants