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

Run integration tests against a Postgres container (Testcontainers) #135

Merged
merged 6 commits into from
Mar 26, 2023

Conversation

Dalet
Copy link
Contributor

@Dalet Dalet commented Mar 5, 2023

Motivation

Integration tests against an in-memory db are not reliable: some SQL requests can work perfectly against the in-memory db and fail on the real database. This PR aims to make running the tests against a real database very simple (no more docker-compose).

Changes summary

  • Changed the README to mention the Docker dependency and how to run in-memory tests

Test project:

  • Made tests run against a temporary Postgres Docker container via the Testcontainers library
    • The container is created only once and automatically deleted at the end.
    • The database is reset by dropping the db and recreating it from an already seeded template DB.
  • Changed the setup of some test classes to run only once for faster execution time.

CI/build changes:

  • Removed db-test from docker-compose because it is replaced by Testcontainers (and its associated POSTGRES_TEST_PORT env variable from example.env
  • Made .env file completely optional (.csproj edits), and removed the step that creates it in the CI.
  • Separated nuget restore, build, and test steps on the CI so it is easier to see where failure occurs.
  • Removed in-memory tests (it's still possible to run them with this env variable: INTEGRATION_TESTS_DB_BACKEND=InMemory)

New dependencies:

Performance

I've made resetting the database as fast as I could, but keep in mind resetting the db is slow, and often easily avoidable.

API requests in [SetUp] also take a lot of time if there are a lot of tests in the class, so they should be kept to a minimum as well.

@Dalet Dalet requested a review from a team as a code owner March 5, 2023 15:47
Copy link
Collaborator

@RageCage64 RageCage64 left a comment

Choose a reason for hiding this comment

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

If we always have to do the same setup in every test fixture, can we make a base class? Maybe something like APIResourceTestFixture?

README.md Outdated Show resolved Hide resolved
@Dalet
Copy link
Contributor Author

Dalet commented Mar 7, 2023

If we always have to do the same setup in every test fixture, can we make a base class? Maybe something like APIResourceTestFixture?

Absolutely, I think it's a good idea but outside the scope of the PR.

zysim
zysim previously approved these changes Mar 11, 2023
@RageCage64
Copy link
Collaborator

If we always have to do the same setup in every test fixture, can we make a base class? Maybe something like APIResourceTestFixture?

Absolutely, I think it's a good idea but outside the scope of the PR.

I personally think it's in scope since this is the PR that adds the setup that matches a clear potential abstraction. But I don't feel strongly enough to block anything.

@zysim
Copy link
Collaborator

zysim commented Mar 26, 2023

If we always have to do the same setup in every test fixture, can we make a base class? Maybe something like APIResourceTestFixture?

Absolutely, I think it's a good idea but outside the scope of the PR.

I personally think it's in scope since this is the PR that adds the setup that matches a clear potential abstraction. But I don't feel strongly enough to block anything.

Let's just make that change in its own PR. Don't foresee any clashes with other PRs and stuff.

@zysim zysim merged commit ca11be5 into leaderboardsgg:main Mar 26, 2023
@Dalet Dalet deleted the testcontainers branch March 26, 2023 15:39
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.

3 participants