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

[2398] Reduce test run times by preventing migration after each test #2399

Merged
merged 6 commits into from
May 24, 2024

Conversation

timyates
Copy link
Collaborator

@timyates timyates commented May 20, 2024

This commit leverages Micronaut TestResources to manage the postgres database container for the tests.

This PR shares the container in TestContainerSuite between all sub-classes.

It fixes the TestPropertyProvider usage, and makes use of the TestContainers singleton pattern

On my maching the tests go from 4m45s to 3m20s (30% speedup)

This commit leverages Micronaut TestResources to manage the postgres database container for the tests.

This means that the container will exist across all the tests and so we save time waiting for it to start up and migrate each test class.

Locally I have seen the time to run all the tests drop from 4m55s to 2m11s
@timyates timyates linked an issue May 20, 2024 that may be closed by this pull request
@timyates timyates self-assigned this May 20, 2024
Copy link
Collaborator

@mjperry91 mjperry91 left a comment

Choose a reason for hiding this comment

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

This looks great to me. It does look like the tests are having issues connecting to the DB on the CI if you haven't already seen it yet.

I was curious how this change ensures the container is shared across all tests. I figured that static block where the container was originally initialized would've been shared as well. Not too familiar with how test resources are working under the hood for Micronaut though, but this certainly would be far simpler for my future reference.

Edit: I suppose because @MicronautTest is spinning up a new application context each time so the static doesn't actually stick?

@ZacharyKlein ZacharyKlein self-requested a review May 21, 2024 04:21
@timyates
Copy link
Collaborator Author

Ahhh... the failures are because I was running the local db (via docker-compose) and now that we're not specifying the URL in the test yaml, it's falling back to using the URL from application.yml in the non-test source... This is looking for a database on localhost:5432 -- which doesn't exist (when you aren't running docker compose up locally) :-(

@timyates
Copy link
Collaborator Author

Micronaut Test Resources works by catching unknown properties at run time, and firing up a container if it can support the property evaluation. These containers by default last the length of the build, and are closed when it completes.

So if a datasource is required, but datasources.default.url is not defined, then the test resources server is queried, and based on the datasources.default.dbType config, a database container is started, and the url is resolved.

However I didn't spot that datasources.default.url has a value in application.yml (it is set to the JDBC_URL environment variable or falls back to a localhost connection url), so TestResources is never queried for a value for the property, so a container isn't started. It was working for me locally as I had a database running... 🙄

So instead, to speed up the tests

  1. I fixed the TestLifecycle so that the properties are resolved in TestContainerSuite based tests.
  2. Removed the @TestContainers and @Container annotations... These will cycle the container after each class
  3. Follow the Singleton container pattern so we just have a single container for all the TestContainersSuite based tests

We still have quite a few tests that do not extend TestContainerSuite and these fire up a new container... I wonder if they should extend the suite instead

@timyates
Copy link
Collaborator Author

Front end snapshot tests are failing... Looks like they're failing on develop too after d9e2acd

Copy link
Collaborator

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Good work. On the same hardware it got from 6 to 4 minutes.

@mjperry91
Copy link
Collaborator

@timyates Thanks for the additional explanation. It sounds like if we just removed the url configuration from application.yml, and into application-dev.yml it would allow you to go with your original approach? This is great though, and I don't think that is something we need to deal with today by any means. I'm not too familiar with how they are configuring their gcloud environments.

@timyates
Copy link
Collaborator Author

It sounds like if we just removed the url configuration from application.yml, and into application-dev.yml it would allow you to go with your original approach?

Yes, or we could remove the configuration totally, and test-resources would also spin up a database automatically when we ran it with ./gradlew server:run.

However I'm also not sure how the database is configured in prod, and I got scared to touch the config there 😉

@mkimberlin mkimberlin merged commit e9b35f8 into develop May 24, 2024
1 check passed
@mkimberlin mkimberlin deleted the feature-2398/use-test-resources branch May 24, 2024 16:38
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.

Reduce build times by reusing the Postgres container between tests
5 participants