Add getR2dbcUrl Method on MariaDB, MySQL, PostgreSQL, MsSQL #9569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
modules/mariadb
modules/mysql
modules/sql-server
modules/postgres
labels
|
Can you add tests, please? |
|
Of course! I'm currently writing and testing the cases. However, I encountered an error in MsSQL Test. Project error log: MsSQL Docker container error log: I've found that these error logs keep repeating(but it seems like it's working fine). After researching, it seems that this issue is not only present in "Testcontainers" (#3079) but also appears in "mssql-docker" issues. However, I am unsure how to resolve this. Could you provide guidance on how to address this problem? whole error log (MsSQL Docker container) |
|
@eddumelendez |
eddumelendez
reviewed
Dec 5, 2024
Comment on lines
29
to
43
| //given | ||
| MariaDBContainer<?> container = createContainer(); | ||
| container.start(); | ||
|
|
||
| String expectedUrl = | ||
| "r2dbc:mariadb://" + | ||
| container.getHost() + | ||
| ":" + | ||
| container.getMappedPort(MariaDBContainer.MARIADB_PORT) + | ||
| "/" + | ||
| container.getDatabaseName() + | ||
| container.constructUrlParameters("?", "&"); | ||
|
|
||
| //when | ||
| String r2dbcUrl = MariaDBR2DBCDatabaseContainer.getR2dbcUrl(container); | ||
| //then | ||
| assertThat(expectedUrl).isEqualTo(r2dbcUrl); | ||
| //clean-up | ||
| container.stop(); |
There was a problem hiding this comment.
avoid comments such as //given, //when, //then, //cleanup
I think the test such test connectivity, performing a query would be enough
There was a problem hiding this comment.
@eddumelendez
I think you're right. I just removed the comments. Thanks for the review!
There was a problem hiding this comment.
Can you update the test performing a query?
Also, use try-with-resources and remove container.close().
There was a problem hiding this comment.
@eddumelendez
Could you please check the code that I have updated to reflect the requested changes?
DHKIM-0511
force-pushed
the
main
branch
2 times, most recently
from
ccfdbc3 to
c070804
Compare
| ConnectionFactory connectionFactory = ConnectionFactories.get(getOptions(container)); | ||
| runTestQuery(connectionFactory); | ||
|
|
||
| String r2dbcUrl = PostgreSQLR2DBCDatabaseContainer.getR2dbcUrl(container); |
There was a problem hiding this comment.
the r2dbcUrl should be used to perform the query. Otherwise is not being tested as it is intended.
There was a problem hiding this comment.
@eddumelendez
I accidentally clicked "discard" earlier, and the PR got closed. I’ll quickly work on reopening it. Sorry for the confusion.
On another note, I initially thought you wanted getR2dbcUrl() to return the original format like r2dbc:mysql://localhost:3306/databasename.
However, if you intend to execute queries directly, I believe the URL should be returned in the "test" format, like r2dbc:tc:mysql:///databasename?TC_IMAGE_TAG=8.0.36. Is that correct?
If not, should we transform it and use the "test" format instead, like the following code?
String r2dbcUrl = MySQLR2DBCDatabaseContainer.getR2dbcUrl(container);
String testUrl = toTestUrl(r2dbcUrl);
ConnectionFactory connectionFactory = ConnectionFactories.get(testUrl);
runTestQuery(connectionFactory);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have added functionality to support the getR2dbcUrl method for MySQL, MariaDB, PostgreSQL, and MsSQL, as it was requested in this issue #8797