-
Notifications
You must be signed in to change notification settings - Fork 632
UN-3636 [MISC] enable the unit-workers rig group and de-flake its suite #2176
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
base: main
Are you sure you want to change the base?
Changes from all commits
503c177
0d1d7e2
f66b9db
2a1acc6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,47 @@ | |
| time if INTERNAL_API_BASE_URL is not set. | ||
| """ | ||
|
|
||
| import os | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from dotenv import load_dotenv | ||
|
|
||
| _env_test = Path(__file__).resolve().parent.parent / ".env.test" | ||
| load_dotenv(_env_test) | ||
|
|
||
| # Worker Celery apps build a Postgres result backend from DB_*/CELERY_BACKEND_DB_*. | ||
| # Strip these before any app is imported so tests don't reach (or leak) a live DB | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit / maintainability: the root conftest does the exact opposite of this block, four lines at a time.
os.environ.setdefault("DB_HOST", "localhost")
os.environ.setdefault("DB_USER", "test")
os.environ.setdefault("DB_PASSWORD", "test")
os.environ.setdefault("DB_NAME", "testdb")Two adjacent conftests with opposing intent on the same four vars, resolving correctly only because pytest loads the root one first. To be clear, the pop is the right mechanism — keep it. It's the only thing that neutralises an ambient exported But those four
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept the pop and cross-referenced both conftests (f66b9db). One correction: those |
||
| # the unit tier has no server for; eager results then stay in-memory. This undoes | ||
| # the DB_* defaults set in ../conftest.py, which stay in effect for shared/tests | ||
| # (not covered by this file). | ||
| for _var in ( | ||
| "DB_HOST", | ||
| "DB_PORT", | ||
| "DB_NAME", | ||
| "DB_USER", | ||
| "DB_PASSWORD", | ||
| "CELERY_BACKEND_DB_HOST", | ||
| "CELERY_BACKEND_DB_PORT", | ||
| "CELERY_BACKEND_DB_NAME", | ||
| "CELERY_BACKEND_DB_USER", | ||
| "CELERY_BACKEND_DB_PASSWORD", | ||
| ): | ||
| os.environ.pop(_var, None) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def _restore_current_celery_app(): | ||
| """Pin celery's default app as current_app around each test. Worker modules | ||
| build their own apps at import and set them current, so `@worker_task` proxies | ||
| otherwise fail to resolve (`NotRegistered`) against a drifted current_app. | ||
| Finalize so every shared task is registered on it. | ||
| """ | ||
| from celery._state import default_app | ||
|
|
||
| default_app.finalize() | ||
| default_app.set_current() | ||
| try: | ||
| yield | ||
| finally: | ||
| default_app.set_current() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mechanism that hid this bug is still in place.
Flipping the marker fixes this group, but not the reason a zero-collect group reported green.
tests/rig/cli.py:52has_NON_FAILING_PYTEST_EXIT_CODES = (0, 5), and pytest exit 5 means "no tests collected". The aggregation atcli.py:481-486:The
not group.optionalguard is never reached for exit 5, so a non-optional group that collects nothing folds in as a pass. That is precisely whatunit-workersdid.tests/rig/is untouched by this PR, so the next marker typo, renamed marker, or moved directory goes green the same way.Worth knowing: the rig already has a guard built for exactly this.
_coverage_attesting_groups()(cli.py:664) excludesemptygroups from attesting coverage, with the comment "a broken marker expression would otherwise report OK with zero tests run". It never fired here only because it applies to groups named incritical_paths.yamlcovered_by, andunit-workersis in none of them.Cheapest close: add
unit-workersto a critical path'scovered_byso the existing gate does its job. Note a blanket_NON_FAILING_PYTEST_EXIT_CODES = (0,)is not safe — hurl groups synthesizeexit 5atcli.py:897, and--marker/--pathsoverrides legitimately zero-collect.Happy for this to be a follow-up ticket if you'd rather keep the PR scoped, but as it stands UN-3636 fixes the instance and leaves the class open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the class-level gap. The fix lives in
tests/rig/(exit-5 handling), which this PR deliberately does not touch, and as you note a blanket(0,)is unsafe given hurl’s synthesized exit 5 and legitimate marker/path zero-collects. Taking your offer to track it as a follow-up rather than widen scope — will file a ticket for a rig-level empty-collect guard on non-optional groups.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to close the class here rather than defer.
2a1acc63adds a runtime gate: a non-optional pytest group that collects zero tests (exit 5) now failsoverall_exitinstead of folding in green. Exemptions match the legit zero-collects you called out —optionalgroups, hurl (its synthesized exit 5 = "no files"), and dev runs with a--marker/--pathsoverride. So_NON_FAILING_PYTEST_EXIT_CODESstays(0, 5)for those; the new guard only fires for the misconfiguration case. Two rig self-tests added (test_empty_nonoptional_group_fails_build,test_empty_group_with_marker_override_does_not_fail).