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 DB unit tests #3186

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Kehrlann
Copy link

@Kehrlann Kehrlann commented Dec 10, 2024

Context

@fhanik and I have noticed that some unit tests did not respect the profile that was passed in, and always ran against an HSQL database, or were ignored

When you ran:

./gradlew '-Dspring.profiles.active=postgresql' test

without a postgres database running, the tests would pass, which should not be the case when a database is involved.

This PR only addresses tests, by fixing this issue, fixing failing tests and adding tooling.

Fixes

The tests in question:

  • Any test with @WithDatabaseContext ; because that annotation had @ActiveProfiles("default"). We removed it.
  • TableAndColumnNormalizationTest, again annotated with @ActiveProfiles("default")
  • And possibly some, but not all, the tests annotated with @DefaultTestContext

When we re-enabled the multi-profile support, we had issues in some tests, notably with Postgres. We have fixed those issues.

Addition: scripts/docker-compose.yml

We added a docker compose file to have MySQL and Postgres running locally. Using run-unit-tests.sh was not working well on our ARM-based macs. This is not expected to be the final setup, but a first step to help us run tests fast.

Addition: @EnabledWithProfile / @DisabledWithProfile

To selectively turn off some tests based on profiles ; instead of relying on custom use of Assume.assumeXXX and Assumptions.assumeXXX which are harder to locate in the code.

See the javadoc for their usage.

Addition: @WithTimeZone

MySQL + the JDBC driver have issues with default time zones. We added an extension to hardcode the timezone to UTC in some unit tests, similar to what JUnit Pioneer does.

fhanik and others added 9 commits December 10, 2024 16:23
1. mysql on port 3306
2. postgresql on port 5432
3. ldap on port 389 and 636 (ssl)
search for `@Disabled` to find tests commented out that don't work for MySQL
- Useful in database testing, allows enabling/disabling tests based on whether `postgresql` or `mysql`
  is present in the list of active profiles.
- The MySQL image is in UTC, but if you run the tests from a machine on different
  timezone, conversions are made inconsistently depending on the datatype used in
  the JDBC queries.
- Therefore, some tests have trouble fetching things "in the past", or may mark
  some tokens as "expired".
- This fixes the tests by hardcoding the timezone to UTC for the specific test.
@Kehrlann Kehrlann marked this pull request as ready for review December 10, 2024 15:34
@Kehrlann Kehrlann marked this pull request as draft December 10, 2024 15:36

services:
postgres:
image: "postgres:17.2"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

PR still in Work in Progress mode

We wrote this on MacOS, I see failures on Linux. Working on those today

fhanik and others added 5 commits December 10, 2024 13:59
Test that fail: JdbcApprovalStoreTests.deleteProviderDeletesApprovals
Why:
 - Table users has column id char(30)
 - Table authz_approvals has column user_id varchar(30)

These two don't match when joining in a query

Suspect: https://dev.mysql.com/worklog/task/?id=12129
For example, joining authz_approvals.user_id is varchar fails when joining against a padded char column
…keyword in SQL

Now we can use MySQL 8
Downgrade to PostgreSQL 15 (need to find out exactly what the test matrix should be)
@fhanik
Copy link
Contributor

fhanik commented Dec 11, 2024

New findings

  1. Tests have database connection leaks. Many tests use JdbcTemplate.getDataSource().getConnection() and never call connection.close()

  2. The @WithTimeZone cause some tests to fail. Need to be investigated. For now, this annotation is limited to mysql

  3. Columns have mismatched data types causing failure in later versions of MySQL (users.id is a padded char while authz_approvals.user_id is a varchar.

  4. Because tests didn't run against MySQL or PostgreSQL, we found many places where the word groups was used in queries and cause BadSqlGrammarException

  5. All the MockMvc tests also had hardcoded to use in memory HSQLDB. This was fixed.

  6. MySQL has in later versions deprecated PAD_CHAR_TO_FULL_LENGTH so we have converted users.id and oauth_client_details.created_by to VARCHAR

Items Left to Do

  1. We can add maildev to docker-compose.yml (not to conflict with the in memory SMTP server SimpleSmtpServer that the integration tests run
  2. We should enhance the LDAP image in docker-compose.yml to properly configure and seed the LDAP server - see scripts/integration-tests.sh
  3. RateLimiter for cargo is causing 429 error on my Macbook M4
  4. /etc/hosts requires the following entrie
127.0.0.1 testzone1.localhost
127.0.0.1 testzone2.localhost
127.0.0.1 testzone3.localhost
127.0.0.1 testzone4.localhost
127.0.0.1 testzonedoesnotexist.localhost
127.0.0.1 oidcloginit.localhost
127.0.0.1 testzoneinactive.localhost
  1. Verify that hsqldb isn't unintentionally used in any tests

  2. Fix remaining integration tests - current status has IT failures (assuming rate limits have been adjusted) - and one of them relates to LDAP preloading data (bullet 2 above) LdapIntegrationTests.test_LDAP_Custom_User_Attributes_In_ID_Token

  3. is CHAR to VARCHAR a breaking change?

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

Successfully merging this pull request may close these issues.

3 participants