Run tests in parallel with pytest-xdist (PP-1212) #1817

jonathangreen · 2024-05-01T19:22:01Z

Description

Uses pytest-xdist to bring down the time it takes to run our test suite. It takes the time in CI from 15 minutes to 7 minutes or so. Locally for me it brings a run down to about 3 minutes, which is much easier to deal with then the 13 minutes that it used to take.

Because this involves running the tests in parallel, some of the test fixtures needed to be updated to allow multiple workers to be calling them at the same time. The biggest change here is that the tests used to run in the database name that is passed into the tests, but now the tests create their own database, then clean it up afterwords, so that each worker has its own database.

Motivation and Context

Make it less painful to run our test suite and debug problems.

How Has This Been Tested?

Running locally
Running in CI

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-05-01T19:33:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (c733da5) to head (aee08a4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1817      +/-   ##
==========================================
+ Coverage   90.03%   90.05%   +0.01%     
==========================================
  Files         299      299              
  Lines       39543    39523      -20     
  Branches     8588     8582       -6     
==========================================
- Hits        35604    35592      -12     
+ Misses       2613     2610       -3     
+ Partials     1326     1321       -5

Flag	Coverage Δ
manager	`?`
migration	`?`

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.

jonathangreen · 2024-05-03T14:02:16Z

.github/workflows/test.yml

-          name: manager-${{ matrix.python-version }}
-          verbose: true
-
-  test-migrations:


Removed because migrations are now run as part of the main test suite.

jonathangreen · 2024-05-03T14:02:57Z

pyproject.toml

 relative_files = true
-source = ["palace.manager"]
+source = ["src"]


This needed to be updated to get coverage and xdist working together.

jonathangreen · 2024-05-03T14:04:28Z

src/palace/manager/core/config.py

-        else:
-            environment_variable = cls.DATABASE_PRODUCTION_ENVIRONMENT_VARIABLE
-
-        url = os.environ.get(environment_variable)


Setting up the DB for testing is entirely handled by the pytest mocks now and doesn't depend on the TESTING env var.

jonathangreen · 2024-05-03T14:06:01Z

src/palace/manager/search/service.py

-    @abstractmethod
-    def indexes_created(self) -> list[str]:
-        """A log of all the indexes that have been created by this client service."""
-


This method was only ever used in tests to delete created indexes. This is now handled by the search fixture, so this method can come out.

jonathangreen · 2024-05-03T14:07:22Z

src/palace/manager/sqlalchemy/session.py

-                # If a factory is being created from a session in test mode,
-                # use the same Connection for all of the tests so objects can
-                # be accessed. Otherwise, bind against an Engine object.
-                bind_obj = bind_obj.engine


This was moved to pytest fixtures as well, so we no longer have to rely on TESTING env var.

jonathangreen · 2024-05-03T14:26:16Z

tests/manager/sqlalchemy/test_session.py

+        assert timestamp is not None
+        old_timestamp = timestamp.finish
+        SessionManager.initialize_data(db.session)
+        assert old_timestamp == timestamp.finish


This first test is just moved from tests/manager/sqlalchemy/test_util.py since this is a more appropriate location for it.

jonathangreen · 2024-05-03T14:27:31Z

tests/manager/sqlalchemy/test_session.py

+    session = production_session()
+    mock_database_url.assert_called_once()
+    mock_session.assert_called_once_with("test-url", initialize_data=True)
+    assert session == mock_session.return_value


Since the DB fixture mocks these for most tests now, add some small tests for these functions to make sure they are doing what we expect and to maintain coverage of them.

jonathangreen · 2024-05-03T14:29:29Z

tests/fixtures/services.py

@@ -201,7 +201,7 @@ def wired(self) -> Generator[None, None, None]:
        self.services.unwire()


-@pytest.fixture(autouse=True)
+@pytest.fixture


This fixture was set to autouse before, but that isn't necessary. It just needs to be added to the alembic fixture. I'd like to avoid autouse fixtures if we can, so you always know the fixtures that are active when calling a function.

jonathangreen · 2024-05-03T14:31:22Z

tests/migration/conftest.py



 @pytest.fixture
-def alembic_engine(database: DatabaseFixture) -> Engine:
+def alembic_engine(function_database: DatabaseFixture) -> Engine:


Now that we have a fixture function_database that can provide a per test function database in the main test suite, we can use it here to run the alembic tests in an isolated environment as part of the main test suite.

jonathangreen · 2024-05-03T14:32:10Z

tests/migration/conftest.py



 @pytest.fixture
 def alembic_runner(
    alembic_config: dict[str, Any] | alembic.config.Config | Config,
    alembic_engine: Engine,
+    services_fixture: ServicesFixture,


services_fixture was added here, so that it no longer needs to be an autouse fixture.

tdilauro

Looks great and is a huge time saver! 🚀

A couple of minor comments, one of which is not even related to this PR, per se.

tdilauro · 2024-05-03T15:14:53Z

src/palace/manager/core/config.py

        try:
            url_obj = make_url(url)
        except ArgumentError as e:
-            # Improve the error message by giving a guide as to what's
-            # likely to work.
+            # Improve the error message by giving a guide as to what's likely to work.
            raise ArgumentError(
                "Bad format for database URL (%s). Expected something like postgresql://[username]:[password]@[hostname]:[port]/[database name]"


I know this isn't a change in this PR, but just noticed that this could leak db credentials into the logs.

I can update that as part of this PR. Does just dropping the (%s) part from the string make sense?

tdilauro · 2024-05-03T16:18:46Z

docker-compose.yml

@@ -22,7 +22,7 @@ x-cm-variables: &cm
    PALACE_CELERY_CLOUDWATCH_STATISTICS_DRYRUN: "true"

    # Set up the environment variables used for testing as well
-    SIMPLIFIED_TEST_DATABASE: "postgresql://palace:test@pg:5432/circ"
+    PALACE_TEST_DATABASE_URL: "postgresql://palace:test@pg:5432/circ"


Minor: Do we want to omit the database name part from this URL, since we're running tests in parallel?

This does work generally, but we can't do it in this case. We connect to whatever DB is passed into the env var, to create the worker DBs. If no DB is passed in, PG defaults to connecting to a DB with the same name as the users. But in this case when we create the container, we tell PG to name the users DB something different, circ instead. So if we don't pass in circ here, the calls will fail with an error something like Unable to connect DB palace does not exist.

jonathangreen force-pushed the feature/parallel-test branch from 993aa79 to 9488ba1 Compare May 1, 2024 19:24

jonathangreen force-pushed the feature/parallel-test branch from 14c3572 to 182efbc Compare May 3, 2024 13:16

Run tests using pytest-xdist to speed up our test runs.

97c086f

jonathangreen force-pushed the feature/parallel-test branch from 182efbc to 97c086f Compare May 3, 2024 14:01

jonathangreen commented May 3, 2024

View reviewed changes

jonathangreen marked this pull request as ready for review May 3, 2024 14:40

jonathangreen requested a review from a team May 3, 2024 14:40

jonathangreen mentioned this pull request May 3, 2024

Remove AUTOINITIALIZE env variable #1824

Merged

2 tasks

Merge branch 'main' into feature/parallel-test

73d3f0e

tdilauro approved these changes May 3, 2024

View reviewed changes

Code review feedback.

aee08a4

jonathangreen merged commit d642e9f into main May 3, 2024
20 checks passed

jonathangreen deleted the feature/parallel-test branch May 3, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests in parallel with pytest-xdist (PP-1212) #1817

Run tests in parallel with pytest-xdist (PP-1212) #1817

jonathangreen commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

jonathangreen May 3, 2024 •

edited

Loading

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

jonathangreen May 3, 2024

tdilauro left a comment

tdilauro May 3, 2024

jonathangreen May 3, 2024

tdilauro May 3, 2024

jonathangreen May 3, 2024

Run tests in parallel with pytest-xdist (PP-1212) #1817

Run tests in parallel with pytest-xdist (PP-1212) #1817

Conversation

jonathangreen commented May 1, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented May 1, 2024 • edited Loading

Codecov Report

jonathangreen May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented May 1, 2024 •

edited

Loading

codecov bot commented May 1, 2024 •

edited

Loading

jonathangreen May 3, 2024 •

edited

Loading