BA-3363: Run Tests in parallel to shorten test run time - #448
Conversation
WalkthroughThis PR adds pytest controls for targeted serial runs, documents parallel test execution, strengthens report GraphQL tests with deterministic data, and defines guidance for hermetic tests under ChangesParallel pytest workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PytestCLI
participant pytest_cmdline_main
participant xdist
PytestCLI->>pytest_cmdline_main: provide targeted node IDs
pytest_cmdline_main->>xdist: set zero workers and disable distribution
xdist->>PytestCLI: run targeted tests serially
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared pytest plugin intended to support parallel-by-default test execution (via pytest-xdist) while keeping targeted single-test runs serial for easier debugging, and updates a flaky GraphQL test to be hermetic under parallel execution.
Changes:
- Add a
pytest11entry point to auto-load a newbaseapp_corepytest plugin. - Update
baseapp_reportsGraphQL query tests to create their ownReportTyperows and assert by membership instead of global counts. - Update agent skill documentation with guidance for parallel-safe (hermetic) tests and serial debugging workflows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Registers baseapp_core.pytest_plugin as a pytest11 auto-loaded plugin entry point. |
baseapp_reports/tests/test_graphql_queries.py |
Makes report-type listing/filtering tests self-contained to avoid reliance on migration-seeded data. |
baseapp_core/pytest_plugin.py |
Adds a pytest hook intended to disable xdist for targeted node-id runs unless -n is explicitly provided. |
.agents/skills/run-development-commands/SKILL.md |
Documents intended parallel-by-default pytest behavior and how to run/debug tests serially. |
.agents/skills/ensure-test-coverage/SKILL.md |
Adds hermetic-test guidance aimed at preventing xdist-related flakiness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [project.entry-points.pytest11] | ||
| baseapp_core = "baseapp_core.pytest_plugin" | ||
|
|
| def pytest_cmdline_main(config) -> None: | ||
| args = config.invocation_params.args | ||
| passed_n = any(a == "-n" or a.startswith(("-n", "--numprocesses")) for a in args) | ||
| if not passed_n and any("::" in a for a in args): | ||
| config.option.numprocesses = 0 | ||
| config.option.dist = "no" |
| Projects run their suite in parallel by default (``setup.cfg`` ``addopts = ... -n 2 --dist | ||
| loadscope``) so CI and local match. This plugin keeps a *targeted* single-test run serial — |
| **Parallel by default.** `pytest` runs `-n 2 --dist loadscope` (from `setup.cfg`) — matches CI so parallel-only failures reproduce locally; each worker uses its own test DB. Tests must be hermetic (see the `ensure-test-coverage` skill). | ||
| - Debug one test serially (pdb works): `docker compose <run> web pytest -n 0 apps/<app>/tests/test_file.py::test_function` — or just pass a `path::test` node id; `baseapp_core`'s pytest plugin auto-serializes targeted runs. | ||
| - Use all cores on a big machine: `docker compose <run> web pytest -n auto`. |
|
|
||
| ## Parallel-safe tests (required) | ||
|
|
||
| CI **and** local run the suite under `pytest-xdist` (`-n 2 --dist loadscope`, per `setup.cfg`) — each worker gets its **own** test DB. Tests must be **hermetic**: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
baseapp_reports/tests/test_graphql_queries.py (1)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace the database-dependent GraphQL tests in the integration directory.
ReportTypeFactory,ProfileFactory, andgraphql_clientuse the database and GraphQL stack. Move this file tobaseapp_reports/tests/integration/test_graphql_queries.py.As per coding guidelines, database-dependent tests must be organized under an
integration/directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp_reports/tests/test_graphql_queries.py` around lines 64 - 65, Move the database-dependent GraphQL test module containing ReportTypeFactory, ProfileFactory, and graphql_client to the integration test directory as test_graphql_queries.py, preserving its existing tests and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/ensure-test-coverage/SKILL.md:
- Line 76: Update the serial reproduction command in the test-coverage guidance
to run through the established web-container Docker wrapper, while preserving
the existing pytest -n 0 path::test arguments and reproduction workflow.
In `@baseapp_core/pytest_plugin.py`:
- Line 16: Update pytest_cmdline_main to annotate config with the appropriate
pytest configuration type and add a docstring describing its targeted-run
serialization behavior; preserve the existing implementation logic.
- Around line 15-21: Add regression tests for pytest_cmdline_main covering
targeted node runs, explicit -n and --numprocesses options, full serial runs,
--deselect arguments containing ::, and PYTEST_ADDOPTS precedence. Verify the
hook sets numprocesses to 0 and dist to no only for eligible targeted runs
without explicit parallel configuration, while preserving existing options in
all other cases.
- Line 19: Update the targeted-run detection condition in the pytest
configuration logic to inspect config.args instead of
config.invocation_params.args, ensuring only parsed collection positional
arguments containing "::" disable parallel execution while raw options such as
--deselect do not.
In `@baseapp_reports/tests/test_graphql_queries.py`:
- Around line 92-94: Update the GraphQL request in the test using
REPORT_TYPES_LIST_GRAPHQL to invoke target_profile.relay_id() when assigning
variables["targetObjectId"], ensuring the declared String variable receives the
Relay ID value rather than the bound method.
---
Nitpick comments:
In `@baseapp_reports/tests/test_graphql_queries.py`:
- Around line 64-65: Move the database-dependent GraphQL test module containing
ReportTypeFactory, ProfileFactory, and graphql_client to the integration test
directory as test_graphql_queries.py, preserving its existing tests and
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a62ff32-e8ec-48bb-8feb-e0f236c9da01
📒 Files selected for processing (5)
.agents/skills/ensure-test-coverage/SKILL.md.agents/skills/run-development-commands/SKILL.mdbaseapp_core/pytest_plugin.pybaseapp_reports/tests/test_graphql_queries.pypyproject.toml
| assert "qa_sub" not in keys # independent of ambient data | ||
| ``` | ||
|
|
||
| Create your own rows (factories + explicit keys); assert **membership**, not global counts. **Reproduce any parallel failure serially before deciding it's real:** `pytest -n 0 path::test`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run the serial reproduction command inside the web container.
The document requires all pytest commands to run in Docker, but this line shows bare pytest -n 0 path::test. Use the same container wrapper defined above.
Proposed fix
-Reproduce any parallel failure serially before deciding it's real: `pytest -n 0 path::test`.
+Reproduce any parallel failure serially before deciding it's real: `docker compose <run> web pytest -n 0 path::test`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/ensure-test-coverage/SKILL.md at line 76, Update the serial
reproduction command in the test-coverage guidance to run through the
established web-container Docker wrapper, while preserving the existing pytest
-n 0 path::test arguments and reproduction workflow.
| @pytest.hookimpl(tryfirst=True) | ||
| def pytest_cmdline_main(config) -> None: | ||
| args = config.invocation_params.args | ||
| passed_n = any(a == "-n" or a.startswith(("-n", "--numprocesses")) for a in args) | ||
| if not passed_n and any("::" in a for a in args): | ||
| config.option.numprocesses = 0 | ||
| config.option.dist = "no" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -e py . | xargs -r rg -n -C 3 \
'pytest_cmdline_main|numprocesses|--deselect|PYTEST_ADDOPTS'Repository: silverlogic/baseapp-backend
Length of output: 791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files relevant to pytest/plugin/tests:\n'
git ls-files | rg '(^|/)(pytest|conftest).*\.py$|(^|/)(pytest|test_|tests)_' || true
printf '\npytest_plugin.py:\n'
cat -n baseapp_core/pytest_plugin.py
printf '\nPotential test files mentioning pytest plugin/dist options:\n'
git ls-files | rg '\.py$' | xargs -r rg -n -C 2 'pytest_cmdline_main|numprocesses|config\.option\.dist|dist|xdist|--deselect|PYTEST_ADDOPTS' || trueRepository: silverlogic/baseapp-backend
Length of output: 27252
Add regression coverage for the targeted-run hook.
The hook exists with no tests covering targeted runs, explicit -n/--numprocesses, full serial runs, --deselect ...::..., or PYTEST_ADDOPTS precedence. Add tests so the behavior cannot regress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_core/pytest_plugin.py` around lines 15 - 21, Add regression tests for
pytest_cmdline_main covering targeted node runs, explicit -n and --numprocesses
options, full serial runs, --deselect arguments containing ::, and
PYTEST_ADDOPTS precedence. Verify the hook sets numprocesses to 0 and dist to no
only for eligible targeted runs without explicit parallel configuration, while
preserving existing options in all other cases.
Source: Coding guidelines
|
|
||
|
|
||
| @pytest.hookimpl(tryfirst=True) | ||
| def pytest_cmdline_main(config) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching pytest_plugin.py:"
fd '^pytest_plugin\.py$' . || true
file="$(fd '^pytest_plugin\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Line count:"
wc -l "$file"
echo
echo "Outline:"
ast-grep outline "$file" || true
echo
echo "Relevant content:"
cat -n "$file"
fi
echo
echo "Search pytest imports/usages in baseapp_core:"
rg -n "pytest|Config|pytest_cmdline_main" baseapp_core || trueRepository: silverlogic/baseapp-backend
Length of output: 23328
Add config’s type annotation and a docstring.
config is untyped, and pytest_cmdline_main has no function docstring despite its non-trivial behavior. Add a docstring that explains the targeted-run serialization; if the docstring is added, the parameter annotation is also required for the same line.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_core/pytest_plugin.py` at line 16, Update pytest_cmdline_main to
annotate config with the appropriate pytest configuration type and add a
docstring describing its targeted-run serialization behavior; preserve the
existing implementation logic.
Source: Coding guidelines
| def pytest_cmdline_main(config) -> None: | ||
| args = config.invocation_params.args | ||
| passed_n = any(a == "-n" or a.startswith(("-n", "--numprocesses")) for a in args) | ||
| if not passed_n and any("::" in a for a in args): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## files"
git ls-files | rg '(^|/)pytest_plugin\.py$|pytest|baseapp_core' | head -200
echo "## target file outline"
ast-grep outline baseapp_core/pytest_plugin.py || true
echo "## target file lines"
cat -n baseapp_core/pytest_plugin.py
echo "## search config.args/passed_n/serial usages"
rg -n 'passed_n|config\.args|any\("::" in|_serial|serial|deselect|pytest_configure|pytest_collection' .Repository: silverlogic/baseapp-backend
Length of output: 50383
🌐 Web query:
pytest 9.0 config.invocation_params.args config.args cmdline parsing --deselect
💡 Result:
In pytest 9.0, config.invocation_params.args provides an immutable snapshot of the raw command-line arguments passed to pytest.main, represented as a tuple of strings [1][2][3]. This attribute is specifically designed to store the exact parameters used during invocation, distinct from the processed configuration options [1][3]. Key technical details regarding these components include: 1. config.invocation_params.args: This attribute is part of the InvocationParams named structure within the Config object [1][4]. It captures the original arguments provided by the user, excluding those loaded from environment variables (like PYTEST_ADDOPTS) or INI files (like addopts), ensuring it remains a faithful record of the specific call [1]. Since pytest 5.4, this has been implemented as an immutable tuple [3]. 2. Command-Line Parsing and --deselect: The --deselect option is a standard pytest feature used to exclude specific tests during collection [5][6]. It is registered via the pytest_addoption hook, allowing users to pass one or more node ID prefixes to ignore [5][6]. When pytest parses command-line arguments, it uses a Parser object that processes these flags into the config.option namespace [7][8]. Specifically, --deselect is stored as a list of strings within config.option.deselect, which can be accessed or modified by plugins during the pytest_collection_modifyitems hook [5][6]. 3. config.args: Unlike invocation_params.args, the config.args attribute is generally used internally by pytest to track the remaining, unprocessed positional arguments (such as file or directory paths) after command-line parsing is complete [9][8]. Plugins and hooks generally should rely on config.invocation_params.args if they need to inspect the original user-provided command string, or config.option for the finalized, parsed configuration state [2][8].
Citations:
- 1: https://github.com/pytest-dev/pytest/blob/e398c93884f228c9808adf9d0be2f876719e703d/src/_pytest/config/__init__.py
- 2: https://stackoverflow.com/questions/72355008/how-do-i-get-all-the-arguments-specified-on-the-command-line-in-a-list
- 3: Make InvocationParams.args a tuple pytest-dev/pytest#6008
- 4: https://docs.pytest.org/en/stable/_modules/_pytest/config.html
- 5: https://docs.pytest.org/en/7.0.x/%5Fmodules/%5Fpytest/main.html
- 6: https://pytest.org/en/7.2.x/_modules/_pytest/main.html
- 7: https://docs.pytest.org/en/latest/_modules/_pytest/config/argparsing.html
- 8: https://docs.pytest.org/en/latest/_modules/_pytest/config.html
- 9: https://fossies.org/linux/misc/pytest-9.0.3.tar.gz/pytest-9.0.3/src/_pytest/config/__init__.py
🌐 Web query:
pytest 9.0.3 src _pytest config __init__ parseopts config.args args_after_parse
💡 Result:
In pytest 9.0.3, the config.args attribute holds the list of command-line arguments (file paths, directory paths, or node IDs) that pytest will collect and execute [1][2][3]. This attribute is populated during the configuration stage after the command-line options have been parsed [2][3]. Key details regarding pytest 9.0.3 and config.args include: 1. Initialization and Population: During the pytest configuration process, the _pytest.config.Config.parse method is called [2]. This method invokes the parser to process command-line options (using _parser.parse_setoption) [2]. After options are extracted, any remaining positional arguments are assigned to config.args [2]. If no arguments are provided, pytest attempts to fall back to the testpaths configuration or the current working directory [2]. 2. Type Constraint: Starting with pytest 9.0.0, config.args is strictly expected to contain strings [4][5]. Previously, it was possible in some versions to pass pathlib.Path objects, but this is no longer supported and may lead to internal errors (e.g., when pytest attempts to partition the arguments) [5]. 3. Non-Public Nature: The config.args attribute is considered a non-public internal API [5]. While it is frequently used by plugins (for example, in the pytest_configure hook to dynamically modify which tests are collected), users should be aware that it may change or be removed in future versions [5][6][3]. 4. Absence of args_after_parse: There is no standard attribute or method named args_after_parse in the pytest 9.0.3 source code under _pytest/config [1][7][2]. The parsing logic is handled primarily through the Parser class and its parse_setoption method [2]. For developers needing to modify collected tests dynamically, overwriting config.args within a pytest_configure hook remains the established, albeit unofficial, method [6][3]. However, plugins should ensure that only strings are used when populating this list to avoid compatibility issues with pytest 9.x [5].
Citations:
- 1: https://fossies.org/linux/misc/pytest-9.0.3.tar.gz/pytest-9.0.3/src/_pytest/config/__init__.py
- 2: https://github.com/pytest-dev/pytest/blob/b7603fa730666cfa86c128d5a0652829582cd930/src/_pytest/config/__init__.py
- 3: https://stackoverflow.com/questions/48235045/pytest-ini-value-not-being-set-correctly
- 4: https://pytest.org/en/latest/changelog.html
- 5: Undocumented breaking change for `str` vs `pathlib.Path` usage between v8.x and v9 in `Config.args`? pytest-dev/pytest#13996
- 6: https://stackoverflow.com/questions/62596631/overriding-collection-paths-via-pytest-plugin
- 7: https://github.com/pytest-dev/pytest/blob/9e9633de9da7a9fab03b4bba3a326bf85b412050/src/_pytest/config/__init__.py
Restrict targeted-run detection to parsed test collection arguments.
config.invocation_params.args includes raw options, so --deselect=tests/test_api.py::test_flaky matches the :: check and forces serial execution. Use config.args for this check so only collection positional arguments can disable parallel runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_core/pytest_plugin.py` at line 19, Update the targeted-run detection
condition in the pytest configuration logic to inspect config.args instead of
config.invocation_params.args, ensuring only parsed collection positional
arguments containing "::" disable parallel execution while raw options such as
--deselect do not.
Assert on tests' own ReportType rows (explicit keys, membership) instead of the migration-seeded defaults, which any TransactionTestCase truncates and pytest-xdist can wipe before these run on a shared worker DB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Projects default to parallel (-n via setup.cfg) so CI and local match. This pytest11 plugin keeps a targeted `path::test` run serial (fast feedback + working pdb) when no explicit -n is given, so single-test debugging isn't slowed by xdist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mands Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ame-scope setup is fine Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8c9eaf0 to
512967f
Compare
…etup.cfg) baseapp-backend configures pytest via pytest.ini, not setup.cfg; reword the plugin docstring and skills so they're accurate for the library (pytest.ini, serial) and consuming projects (setup.cfg, -n enabled) alike. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/ensure-test-coverage/SKILL.md:
- Line 52: Update the pytest configuration references to include pyproject.toml
alongside pytest.ini and setup.cfg in
.agents/skills/ensure-test-coverage/SKILL.md lines 52-52 and
.agents/skills/run-development-commands/SKILL.md lines 67-67, keeping both
documents’ configuration-file lists consistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cc2a437-76c0-4170-8900-b94bceb97ff1
📒 Files selected for processing (5)
.agents/skills/ensure-test-coverage/SKILL.md.agents/skills/run-development-commands/SKILL.mdbaseapp_core/pytest_plugin.pybaseapp_reports/tests/test_graphql_queries.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- baseapp_core/pytest_plugin.py
- baseapp_reports/tests/test_graphql_queries.py
|
|
||
| ## Parallel-safe tests (required) | ||
|
|
||
| Projects that enable `pytest-xdist` run the suite in parallel in CI **and** locally (`-n <N> --dist loadscope`, set in the project's pytest `addopts` — `pytest.ini` or `setup.cfg`), and each worker gets its **own** test DB. Whether or not parallel is enabled, tests must be **hermetic**: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include pyproject.toml in the pytest configuration locations.
Both documents describe addopts as living only in pytest.ini or setup.cfg, but this PR also changes pytest controls in pyproject.toml. This makes the execution guidance incomplete.
.agents/skills/ensure-test-coverage/SKILL.md#L52-L52: listpyproject.tomlwith the other supported pytest configuration files..agents/skills/run-development-commands/SKILL.md#L67-L67: use the same complete configuration-file list.
🧰 Tools
🪛 LanguageTool
[style] ~52-~52: ‘Whether or not’ might be wordy. Consider a shorter alternative.
Context: ...d each worker gets its own test DB. Whether or not parallel is enabled, tests must be **he...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
📍 Affects 2 files
.agents/skills/ensure-test-coverage/SKILL.md#L52-L52(this comment).agents/skills/run-development-commands/SKILL.md#L67-L67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/ensure-test-coverage/SKILL.md at line 52, Update the pytest
configuration references to include pyproject.toml alongside pytest.ini and
setup.cfg in .agents/skills/ensure-test-coverage/SKILL.md lines 52-52 and
.agents/skills/run-development-commands/SKILL.md lines 67-67, keeping both
documents’ configuration-file lists consistent.
Feat/parallel safe tests xdist
Summary by CodeRabbit
Tests
Documentation