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

chore: make integration tests run concurrently #924

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

Conversation

gruuya
Copy link
Contributor

@gruuya gruuya commented Jan 30, 2025

Closes #923

This PR makes integration tests re-use the same set of docker containers, hence allowing them to be run concurrently

  1. all existing tests are moved to a shared module, with a single top-level main.rs file, thus requiring only one compilation step (as opposed to compiling every test file individually
  2. set_test_fixture is made sync, since there's no async code in it
  3. given 1 and 2, the test fixture is wrapped in a OnceLock, so that it is invoked only once for the resulting (shared) test binary; it also has a corresponding destructor to spin down the containers after the tests run
  4. if there's a need for integration tests which don't use the shared docker containers, their test files should be placed alongside main.rs (top-level); otherwise they should be created in the shared module
  5. one (shared) file can now contain an arbitrary number of tests that utilize the shared set of containers (without having to resort to decorating them with #[serial])

Finally, with this change my integration test runs go from taking ~3.5 minutes down to 30 seconds.

pub rest_catalog: RestCatalog,
pub catalog_config: RestCatalogConfig,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the tests now share this fixture but they're all executed in different runtimes, they can't share the same client, as those can get dropped prematurely when a test ends, resulting in dispatch task is gone: runtime dropped the dispatch task

Comment on lines -80 to +81
.name("t1".to_string())
.name("t2".to_string())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the write tests use different tables each allows for them to also use the shared docker stack.

By extension make them use the same set of docker containers.
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

generally lgtm. Since we're using a shared catalog, are there any concerns with side effects?
In pyiceberg we use a table identifier fixture to generate table names so they dont conflict

@gruuya
Copy link
Contributor Author

gruuya commented Jan 30, 2025

generally lgtm. Since we're using a shared catalog, are there any concerns with side effects? In pyiceberg we use a table identifier fixture to generate table names so they dont conflict

Good point, I could also extract the namespace as a shared fixture among the tests (in the PR i just ignore the result of create_namespace assuming it was already created if it was an error).

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.

chore: Improve efficiency of Docker fixtures in integration_tests
2 participants