search

objectcomputing / check-ins

Other
7 stars 7 forks source link

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

Closed timyates closed 1 month ago

timyates commented 1 month ago

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)

timyates commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Front end snapshot tests are failing... Looks like they're failing on develop too after https://github.com/objectcomputing/check-ins/commit/d9e2acd653003dd89216e05726b30a1f0480a1f9

mjperry91 commented 1 month ago

@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 commented 1 month ago

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 😉