Support tc datasource prefix for test containers #19044
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
polarbear567
force-pushed
the
enhancement/tcPrefixForTestContainers
branch
from
fef3237
to
8e529c8
Compare
snicoll
added
type: enhancement
| /** | ||
| * Testcontainers. | ||
| */ | ||
| TESTCONTAINERS("Testcontainers", "org.testcontainers.jdbc.ContainerDatabaseDriver") { |
There was a problem hiding this comment.
is Testcontainers the product name of this driver? The way I understood it was that it is a wrapper for the actual underlying database container
There was a problem hiding this comment.
FYI the driver does not return a product name property from getPropertyInfo.
I would highly recommend to write a test that checks it with the TC's driver (no need to start a container, just the lookup part), especially given that it is already on classpath
There was a problem hiding this comment.
Hi, @snicoll , what should I do?
There was a problem hiding this comment.
In the conversation above we've identified that Test Containers does not have such product name so the current PR is wrong. Also @bsideup was suggesting to write a test for the lookup part. Would you be willing to rework the PR to that effect please?
There was a problem hiding this comment.
Sure, but I hava some questions.
Assuming that the current testcontainers is a pair of mysql packaging, if we dynamically match the produceName to mysql, will it cause confusion? Testcontainers itself does not seem to rename the driver for its packaging.
There was a problem hiding this comment.
Sorry but I don't understand the question. The bottom line here is that test containers does not have a product name and as such the first argument of that enum should be null. Tests should be updated accordingly.
There was a problem hiding this comment.
I thought you mean that there must be a product name for Testcontainers. I've changed it to null now. For test, I'm not sure what @bsideup wants to express, so the lookup part I wrote for test case is for "fromJdbcUrl" method. If there are other changes or suggestions about test, please let me know. Thanks!
snicoll
added
the
status: waiting-for-feedback
snicoll
removed
the
status: waiting-for-feedback
|
Thanks @polarbear567, rather than this back and forth on the test, I've polished your proposal with the additional test, see 4c7896b |
No description provided.