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

chore(deps): Update dependencies #279

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

reneleonhardt
Copy link

No description provided.

Copy link
Collaborator

@tomix26 tomix26 left a comment

Choose a reason for hiding this comment

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

I'm not sure if upgrading everything at once is a good idea, but if you manage to do it successfully, then why not.

I've added a few quick comments to your changes to clarify what needs to be addressed before the pull request can be merged.

@@ -113,7 +113,7 @@
"name": "zonky.test.database.postgres.docker.image",
"type": "java.lang.String",
"description": "Docker image containing PostgreSQL database.",
"defaultValue": "postgres:11-alpine"
"defaultValue": "postgres:16-alpine"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind that such changes will have to wait until the next major version.

"description": "Version of EnterpriseDB PostgreSQL binaries (https://www.enterprisedb.com/download-postgresql-binaries).",
"defaultValue": "11.10-1"
"description": "Version of EnterpriseDB PostgreSQL binaries (https://www.enterprisedb.com/download-postgresql-binaries). 11+ for Linux available on https://www.postgresql.org/download/",
"defaultValue": "10.23-1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The PostgreSQL versions for OpenTable and Yandex providers must match the other providers, regardless of whether they support Linux platform or not. I plan to remove them in the next major version anyway.

@@ -100,9 +101,9 @@ public void afterTestClass(TestContext testContext) {
DatabaseContext databaseContext = applicationContext.getBean(DatabaseContext.class);
DatabaseProvider databaseProvider = applicationContext.getBean("dockerPostgresDatabaseProvider", DatabaseProvider.class);

verify(databaseContext, times(4)).reset();
verify(databaseContext, atLeast(2)).reset();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change could significantly affect the library's performance, so it's necessary to carefully consider and justify every such change.

@@ -51,7 +51,7 @@ public void testConfigurationProperties() throws Exception {

JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource);

String collate = jdbcTemplate.queryForObject("show lc_collate", String.class);
String collate = jdbcTemplate.queryForObject("SELECT datcollate FROM pg_database WHERE datname NOT IN('template0', 'template1', 'postgres') LIMIT 1", String.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to keep the test code as simple as possible. So, unless this change is necessary for some reason, I would prefer the original version.

@@ -77,7 +77,7 @@ public void testGetDatabase() throws Exception {
@Test
public void testConfigurationProperties() throws Exception {
MockEnvironment environment = new MockEnvironment();
environment.setProperty("zonky.test.database.postgres.yandex-provider.postgres-version", "9.6.11-1");
environment.setProperty("zonky.test.database.postgres.yandex-provider.postgres-version", "10.23-1");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the point of this test is to have a different PostgreSQL version from the default value to verify that changing the property works.

[name: '7.6.0', flyway: '7.6.0', 'flyway-test': '7.0.0', spring: '5.3.13', 'spring-boot': '2.4.13', 'zonky-postgres': 'default'],
[name: '7.15.0', flyway: '7.15.0', 'flyway-test': '7.0.0', spring: '5.3.27', 'spring-boot': '2.5.15', 'zonky-postgres': 'default'],
[name: '8.0.5', flyway: '8.0.5', 'flyway-test': '7.0.0', spring: '5.3.27', 'spring-boot': '2.6.15', 'zonky-postgres': 'default'],
// [name: '4.0.3', flyway: '4.0.3', 'flyway-test': '4.0.1', spring: '4.3.30.RELEASE', 'spring-boot': '1.5.22.RELEASE', 'zonky-postgres': 'default'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind that you can freely add new versions to the version matrix, but all original versions must remain functional. I'm referring to both library/framework versions (Flyway, Liquibase, Spring, Spring Boot, ...) as well as database versions (Postgres, MSSQL, MySQL, MariaDB, ...).

api 'org.testcontainers:mssqlserver:1.18.3'
api 'org.testcontainers:mysql:1.18.3'
api 'org.testcontainers:mariadb:1.18.3'
api 'org.testcontainers:postgresql:1.20.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

My intention is to support a wide range of versions at runtime, including the latest ones, but when it comes to default versions, these should be held back a bit. For more details on why I believe using the latest versions in projects like this isn't ideal, check out my previous comment:
#260 (comment)

@@ -327,7 +336,7 @@ project(':embedded-database-spring-test') {

eachDependency { details ->
if (details.requested.group == 'junit') {
details.useVersion "4.12"
details.useVersion "4.13.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just want to warn you in advance that I don't recommend upgrading to JUnit 5 here, because migrating the logic in TestAssumptions isn't trivial. I've tried it at least once before and wasn't successful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants