Skip to content

Replace run-with-docker.sh with testcontainer #301

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicknovitski
Copy link
Member

@nicknovitski nicknovitski commented Jun 23, 2025

Why

In CI it takes the same time, but locally it's usually a lot faster, since the common case is that the docker images are already downloaded, and since each test suite running its own services means the integration tests can be run concurrently, even alongside unit tests.

Generally it's easier to run all the tests with the new command yarn test:all and also simpler to run individual tests, and anything in between.

How

testcontainer is a library made by Docker to handle disposable containers for tests.

It can process docker compose files, and at first I was going to only replace the shell script with global setup js files, but to gain the advantages listed in 'Why`, since there weren't too many involved tests, I added code to each test suite to start and stop its own services. Also, there was no need to track and share ports or connection info.

I think some of this could be moved into entity-testing-utils, but I decided to wait on that.

Test plan

  • docker system prune -a (delete all locally cached images)
  • yarn test:all (downloads images first)
  • yarn test:all (runs with downloaded images in less than 10 seconds)
  • all existing more focused commands in each project work as before.

Copy link
Member Author

nicknovitski commented Jun 23, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

codecov bot commented Jun 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (0038d63) to head (448d75b).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #301   +/-   ##
=======================================
  Coverage   97.39%   97.39%           
=======================================
  Files          87       87           
  Lines       11894    11894           
  Branches     1038      962   -76     
=======================================
  Hits        11584    11584           
- Misses        303      310    +7     
+ Partials        7        0    -7     
Flag Coverage Δ
integration ?
unittest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nicknovitski nicknovitski changed the base branch from root-scripts to graphite-base/301 June 23, 2025 23:26
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from 51c6e40 to 10d14c4 Compare June 23, 2025 23:45
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from 10d14c4 to b0bbb62 Compare June 24, 2025 00:25
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

approveyoda

@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from b0bbb62 to 8147db5 Compare June 24, 2025 23:32
@nicknovitski nicknovitski changed the base branch from graphite-base/301 to main June 24, 2025 23:32
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from 8147db5 to cfd54a3 Compare June 24, 2025 23:46
@wschurman wschurman self-requested a review June 24, 2025 23:55
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch 3 times, most recently from e883c4e to 5fc9b3b Compare June 25, 2025 01:14
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from 5fc9b3b to cfeca43 Compare June 25, 2025 01:50
@nicknovitski nicknovitski marked this pull request as ready for review June 25, 2025 01:58
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

When I run yarn test (or yarn test --selectProjects integration) from the root, a decent number of tests time out. I have a feeling it might be due to each test starting its own container? In general, I worry that this slows down integration tests quite a bit now. I recall during an earlier iteration of this PR it still used a shared testcontainer? (the DockerComposeEnvironment in integration-setup.js)

Copy link
Member

For example:
On this branch, in entity-database-adapter-knex package, yarn integration times out after 60s sometimes, sometimes takes ~30s (including already warmed runs).
On main, it takes ~10s including setup.

I really like the idea of them being able to run concurrently though.

@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch 7 times, most recently from b701e32 to cc926dd Compare June 26, 2025 15:36
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from cc926dd to 4822750 Compare June 26, 2025 15:48
@nicknovitski nicknovitski requested a review from wschurman June 26, 2025 16:01
Each test starts its own service dependencies, transparently downloading
and/or re-using docker images.  This allows all tests to be run in one command.
@nicknovitski nicknovitski force-pushed the 06-23-replace_run-with-docker.sh_with_testcontainer branch from 4822750 to 448d75b Compare June 26, 2025 16:19
@nicknovitski
Copy link
Member Author

There's now a setup file that ensures all used images are pulled before any integration tests are run. This can't time out, but if it fails the error message will be visible. The tests consistently take about 10 seconds on my machine.

Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Couple issues:

  1. When I run yarn integration from the root on my machine (and re-running it, too), most tests seem to time out:
    Test Suites: 10 failed, 5 passed, 15 total
    Tests:       59 failed, 10 passed, 69 total
    Snapshots:   0 total
    Time:        11.9 s, estimated 12 s
    
  2. When I run yarn integration from packages/entity-database-adapter-knex I get Usage Error: Couldn't find a script named "test:integration".

Happy to hop on a call if you're unable to repro the timeouts on your side.

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