Skip to content

Test fixture cleanup#6008

Merged
galvana merged 61 commits intomainfrom
test-cleanup
Apr 23, 2025
Merged

Test fixture cleanup#6008
galvana merged 61 commits intomainfrom
test-cleanup

Conversation

@galvana
Copy link
Copy Markdown
Contributor

@galvana galvana commented Apr 8, 2025

Closes LJ-526

Description Of Changes

General cleanup of tests and fixtures. Tests no longer have implied dependencies or required execution order. This means tests can run independently and more importantly in parallel.

  • Removed most autouse fixtures except for a few essential ones. Each test now explicitly defines its required fixtures.
  • Refactored tests to run independently without requiring a specific test order to pass.
  • A standard clear_db_tables function-scoped fixture to reset the DB state at the end of each test.
  • Removed ctl and ops specific db fixtures in favor of single db and async_session fixtures defined at the root level conftest.py.
  • Refactored migrate_db to only handle DB migration. Side effects of creating seed and sample data were moved to separate functions.
  • Added pytest-xdist and enabled parallel test execution with 4 workers
    • ops-unit-api from 14 minutes to 4.5 minutes
    • ops-unit-non-api from 15 minutes to 4.5 minutes

Steps to Confirm

  1. All tests should pass

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Apr 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2025 11:34pm
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2025 11:34pm

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 16 lines in your changes missing coverage. Please review.

Project coverage is 86.85%. Comparing base (72532df) to head (eb2e09a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/db/seed.py 84.61% 7 Missing and 1 partial ⚠️
src/fides/api/api/deps.py 60.00% 4 Missing ⚠️
src/fides/api/app_setup.py 66.66% 2 Missing and 1 partial ⚠️
src/fides/api/api/v1/endpoints/admin.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6008      +/-   ##
==========================================
- Coverage   86.87%   86.85%   -0.03%     
==========================================
  Files         419      419              
  Lines       25980    26000      +20     
  Branches     2827     2829       +2     
==========================================
+ Hits        22570    22581      +11     
- Misses       2789     2806      +17     
+ Partials      621      613       -8     

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

Comment on lines -100 to -102
"timeout",
"--signal=INT",
"360",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need to timeout anymore, tests don't get stuck

Comment on lines +128 to +129
"-n",
"4",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This flag runs the tests with 4 workers

Comment thread src/fides/api/db/seed.py


def load_default_dsr_policies() -> None:
def load_default_dsr_policies(session: Session) -> None:
Copy link
Copy Markdown
Contributor Author

@galvana galvana Apr 21, 2025

Choose a reason for hiding this comment

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

Made this synchronous to avoid DB locking in the FastAPI lifespan function

Comment thread tests/conftest.py

@pytest.fixture(scope="function")
@pytest.mark.asyncio
async def fideslang_resources(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This fixture creates the data needed for some CLI tests. This makes it so we're not dependent on the side-effects of other tests

Comment thread tests/conftest.py
ApplicationConfig.update_config_set(db, CONFIG)


@pytest.fixture(autouse=True, scope="function")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed autouse to make it easier to see which fixtures a test is dependent on

Comment thread tests/conftest.py
Comment on lines +1876 to +1902
@pytest.fixture(scope="function")
def default_data_categories(db: Session):
for data_category in DEFAULT_TAXONOMY.data_category:
if (
DataCategoryDbModel.get_by(db, field="name", value=data_category.name)
is None
):
DataCategoryDbModel.create(
db=db, data=data_category.model_dump(mode="json")
)


@pytest.fixture(scope="function")
def default_data_uses(db: Session):
for data_use in DEFAULT_TAXONOMY.data_use:
if DataUse.get_by(db, field="name", value=data_use.name) is None:
DataUse.create(db=db, data=data_use.model_dump(mode="json"))


@pytest.fixture(scope="function")
def default_organization(db: Session):
load_default_organization(db)


@pytest.fixture(scope="function")
def default_taxonomy(db: Session):
load_default_taxonomy(db)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Saving some time by not creating these fixtures unless they're needed by a test

Comment thread tests/conftest.py


@pytest.fixture(scope="function", autouse=True)
async def clear_db_tables(db, async_session):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Clears all database tables after each test run. This also closes any open DB connections (sync or async) that may linger after a test

Comment thread tests/conftest.py
Comment on lines +1961 to +1983
def pytest_configure_node(node):
"""Pytest hook automatically called for each xdist worker node configuration."""
if hasattr(node, "workerinput") and node.workerinput:
worker_id = node.workerinput["workerid"]
print(
f"[Configure Node] Configuring database and config for worker {worker_id}..."
)

os.environ["FIDES__DATABASE__TEST_DB"] = f"fides_test_{worker_id}"

get_config.cache_clear()
fides_config = get_config()
sync_db_uri = fides_config.database.sqlalchemy_test_database_uri
async_db_uri = fides_config.database.async_database_uri

# Log connection strings
print(
f"[Configure Node] Sync DB URI: {sync_db_uri} Async DB URI: {async_db_uri}"
)
else:
print(
"[Configure Node] Skipping DB setup/config update on single node or non-xdist run."
)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is what allows for parallel test execution. This runs before any of the tests, and allows each worker to specify the env var for its own test DB. Each worker then creates its own database at the beginning of the test run that it uses for all of its tests.

Comment thread dev-requirements.txt
pytest-env==0.7.0
pytest-mock==3.14.0
pytest-rerunfailures==14.0
pytest-xdist==3.6.1
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For parallel test execution ⚡

Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

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

Really nice work! Very exciting changes!

Comment on lines +173 to +178
configure_db(CONFIG.database.sync_database_uri)
with get_autoclose_db_session() as session:
seed_db(session)
if CONFIG.database.load_samples:
async with get_async_autoclose_db_session() as async_session:
await seed.load_samples(async_session)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

return FidesDataCategory


# TODO: Move away from using this, it conflicts with the DataCategory model
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a story to follow up on this?


@pytest.mark.unit
class TestFilterDataCategories:
@pytest.mark.skip("this times out on CI")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

Comment thread tests/ctl/cli/test_cli.py


class TestDB:
@pytest.mark.skip(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

Comment thread tests/ctl/conftest.py


@pytest.fixture(scope="session")
def monkeysession():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!!

@galvana galvana mentioned this pull request Apr 22, 2025
15 tasks
@cypress
Copy link
Copy Markdown

cypress Bot commented Apr 24, 2025

fides    Run #12866

Run Properties:  status check passed Passed #12866  •  git commit bf97f58e8e: Test fixture cleanup (#6008)
Project fides
Branch Review main
Run status status check passed Passed #12866
Run duration 00m 53s
Commit git commit bf97f58e8e: Test fixture cleanup (#6008)
Committer Adrian Galvan
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run unsafe ci checks Runs fides-related CI checks that require sensitive credentials

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants