chore: use shared containers for integration tests #924

gruuya · 2025-01-30T13:10:19Z

Closes #923

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

all existing tests are moved to a shared_tests module, with a single top-level shared.rs file, thus requiring only one compilation step (as opposed to compiling every test file individually)
set_test_fixture is made sync, since there's no async code in it
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
if there's a need for integration tests which don't use the shared docker containers, their test files should be placed alongside shared.rs (top-level); otherwise they should be created in the shared_tests module
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.

EDIT: The CI unit workflow run duration also seems ~10min shorter now.

gruuya · 2025-01-30T13:15:11Z

crates/integration_tests/src/lib.rs

-    pub rest_catalog: RestCatalog,
+    pub catalog_config: RestCatalogConfig,


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

gruuya · 2025-01-30T13:16:11Z

crates/integration_tests/tests/shared/append_partition_data_file_test.rs

-        .name("t1".to_string())
+        .name("t2".to_string())


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

kevinjqliu

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 · 2025-01-30T16:27:02Z

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).

EDIT: I've extracted two common fixtures now: the apple-ios namespace and the foo-bar-baz schema.

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

xxchan

Not sure if I understand it correctly:

Before this PR, tests are compiled into separated binaries. In theory they can run concurrently (e.g., if use cargo nextest), but just cargo test will run them in serial.
In this PR, we put them into 1 binary. And then they are run concurrently by #[tokio::tests].
- We use "shared container" to also save the time of spinning up and down containers. But need to take care of potential conflicts.

xxchan · 2025-02-02T03:15:11Z

crates/integration_tests/tests/shared_tests/scan_all_type.rs

-    let ns = Namespace::with_properties(
-        NamespaceIdent::from_strs(["apple", "ios"]).unwrap(),
-        HashMap::from([
-            ("owner".to_string(), "ray".to_string()),
-            ("community".to_string(), "apache".to_string()),
-        ]),
-    );
+    let fixture = get_shared_containers();
+    let rest_catalog = RestCatalog::new(fixture.catalog_config.clone());
+    let ns = apple_ios_ns().await;


What about creating a per-test randomized namespace instead? I don't get why do we want a shared namespace. I guess before this PR, we just didn't consider the concurrency and conflict problem.

Yup, that makes sense, I'll change it.

EDIT: Done, the randomness is introduced via uuids.

gruuya · 2025-02-02T06:53:09Z

Before this PR, tests are compiled into separated binaries. In theory they can run concurrently (e.g., if use cargo nextest), but just cargo test will run them in serial.

Correct; at present the tests can't be run concurrently because they depend on separate docker container sets. (Also even if they didn't, cargo nextest would compile each top-level test file separately.)

In this PR, we put them into 1 binary. And then they are run concurrently by #[tokio::tests].

We use "shared container" to also save the time of spinning up and down containers. But need to take care of potential conflicts.

Yep, that's it in a nutshell. Using the shared container set is the biggest time-saver (eliminating the docker build-start-stop overhead for each test). Having made them use shared containers, the next simple improvement is to make them all compile to one test binary (thus eliminating multiple compilations), as is done through having a single top-level shared.rs file, and consequently run them concurrently as well.

xxchan

Code changes and the idea LGTM 👍 Maybe we could rename the PR title to "use shared container for integration tests" instead as it's the biggest time-saver?

kevinjqliu · 2025-02-02T18:41:43Z

thanks! generally lgtm.
Does this also affect CI runs? if so, would be great to also include how much time we saved there too.

gruuya · 2025-02-03T07:48:40Z

thanks! generally lgtm. Does this also affect CI runs? if so, would be great to also include how much time we saved there too.

Good question; a brief glance suggests it may have shaved about ~5 minutes from the unit CI workflow, e.g. by comparing the Test step duration from a job on this PR(3m 51s) and a job on another PR(8m 45s).

The difference would certainly grow as more and more integration tests are added.

EDIT: It's actually closer to 10 minutes, since the integration tests are run twice, once in the Test step and once in the Async-std Test step.

kevinjqliu · 2025-02-03T16:15:23Z

thats amazing! Thanks for looking into that, i like faster CI :)

gruuya commented Jan 30, 2025

View reviewed changes

gruuya force-pushed the concurrent-integration-tests branch from fd569a9 to 110ce33 Compare January 30, 2025 13:17

gruuya mentioned this pull request Jan 30, 2025

chore: Improve efficiency of Docker fixtures in integration_tests #923

Open

kevinjqliu reviewed Jan 30, 2025

View reviewed changes

gruuya added 2 commits January 31, 2025 09:56

Make integration tests run concurrently

0d995da

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

Extract the common namespace and schema as fixtures for the shared tests

7fb4d98

gruuya force-pushed the concurrent-integration-tests branch from fcf379f to 7fb4d98 Compare January 31, 2025 08:56

xxchan reviewed Feb 2, 2025

View reviewed changes

Make shared tests use random namespaces

7587992

xxchan approved these changes Feb 2, 2025

View reviewed changes

gruuya changed the title ~~chore: make integration tests run concurrently~~ chore: use shared containers for integration tests Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use shared containers for integration tests #924

chore: use shared containers for integration tests #924

gruuya commented Jan 30, 2025 •

edited

Loading

gruuya Jan 30, 2025

gruuya Jan 30, 2025

kevinjqliu left a comment

gruuya commented Jan 30, 2025 •

edited

Loading

xxchan left a comment

xxchan Feb 2, 2025

gruuya Feb 2, 2025 •

edited

Loading

gruuya commented Feb 2, 2025 •

edited

Loading

xxchan left a comment

kevinjqliu commented Feb 2, 2025

gruuya commented Feb 3, 2025 •

edited

Loading

kevinjqliu commented Feb 3, 2025

		pub rest_catalog: RestCatalog,
		pub catalog_config: RestCatalogConfig,

chore: use shared containers for integration tests #924

Are you sure you want to change the base?

chore: use shared containers for integration tests #924

Conversation

gruuya commented Jan 30, 2025 • edited Loading

gruuya Jan 30, 2025

Choose a reason for hiding this comment

gruuya Jan 30, 2025

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

gruuya commented Jan 30, 2025 • edited Loading

xxchan left a comment

Choose a reason for hiding this comment

xxchan Feb 2, 2025

Choose a reason for hiding this comment

gruuya Feb 2, 2025 • edited Loading

Choose a reason for hiding this comment

gruuya commented Feb 2, 2025 • edited Loading

xxchan left a comment

Choose a reason for hiding this comment

kevinjqliu commented Feb 2, 2025

gruuya commented Feb 3, 2025 • edited Loading

kevinjqliu commented Feb 3, 2025

gruuya commented Jan 30, 2025 •

edited

Loading

gruuya commented Jan 30, 2025 •

edited

Loading

gruuya Feb 2, 2025 •

edited

Loading

gruuya commented Feb 2, 2025 •

edited

Loading

gruuya commented Feb 3, 2025 •

edited

Loading