-
Notifications
You must be signed in to change notification settings - Fork 74
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
Run all integration tests using a MySQL test container instead of HSQL #813
base: master
Are you sure you want to change the base?
Conversation
2092b03
to
72e37c6
Compare
f1c3255
to
994a651
Compare
Replace HSQL with MySQL for higher fidelity integration tests and to be able to also test native queries and platform-specific functionality.
994a651
to
7e7f5bc
Compare
|
||
|
||
### Testcontainers | ||
By default, if no datasource is configured in the application.properties file, integration tests rely on |
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.
One of the advantage of the current setup is that everything can be running out of the box for simple development. Adding docker as requirement adds complexity and might flakiness/impact perf. I'd keep the default settings to run in memory and just activate container testing with a spring profile that we can use or not locally and in dev. On current CI we could run both tests in parallel at the beginning?
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.
Given that other popular Java frameworks now rely on similar dependencies for testing (e.g.: Quarkus) - I don't have any specific reason to believe that flakiness/perf is going to be significantly impacted by this change.
The biggest value for integration tests is that they find issues locally, before deploying the application live. I believe ensuring the default behaviour is as close to production and finds as many bugs should be "base" state then. (opt-out vs opt-in).
If the target is simplifying setup - we could make the existing setup steps significantly simpler by removing the MySQL install, configuration & application configuration setup steps and replacing that with the requirement to just have Docker instead (+ also changing the default for the local application to use TC).
I would expect it's more likely today to have Docker already installed on one's machine than to find the right version of MySQL that this application depends on - but I wouldn't have any data backing that.
@@ -1,4 +1,7 @@ | |||
info.build.version=@project.version@ | |||
|
|||
spring.flyway.enabled=true | |||
spring.datasource.url=jdbc:tc:mysql:5.7://localhost:3306/mojito?characterEncoding=UTF-8&useUnicode=true&TC_INITSCRIPT=db/mysql/test_init.sql |
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.
will the hardcoded port cause issue?
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.
It won't cause any issues, it's just a placeholder value when Testcontainers is used, see https://www.testcontainers.org/modules/databases/jdbc/ for details.
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>mysql</artifactId> | ||
<version>1.17.1</version> |
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.
should that be a "test" dependency?
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.
This was not made a test dependency explicitly so that someone could decide to configure their local instance to run with TC as well out of the box with a configuration change, without needing to separately install MySQL.
|
||
@Test | ||
public void testIntegrationTestsRunOnMysql() { | ||
Assert.assertTrue(dbUtils.isMysql()); |
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.
that'd fail if we run the test against anything but mysql?
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.
That is the intent - to make sure that integration tests run in an environment that is similar to production, otherwise the tests risk not being as valuable and missing real world issues (as we've seen HSQL has missed several times before).
Replace HSQL with MySQL for higher fidelity integration tests and to
be able to also test native queries and platform-specific functionality.