feat: Add automated pytest slow marker tuner #836
Conversation
- Add CLI tool for automatically tuning pytest slow markers based on test execution time - Add /tune-slow-markers slash command for PR automation - Tool can add slow markers to tests exceeding timeout threshold - Tool can optionally remove slow markers from fast tests - Configurable timeout threshold (default: 7.0s) - Supports dry-run mode for preview - Compatible with pipx run for standalone usage Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1761179606-add-slow-marker-tuner' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1761179606-add-slow-marker-tuner'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Tune Slow Markers | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout PR branch | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| repository: ${{ github.event.client_payload.pull_request.head.repo.full_name }} | ||
| ref: ${{ github.event.client_payload.pull_request.head.ref }} | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | ||
| with: | ||
| python-version: '3.10' | ||
|
|
||
| - name: Set up Poetry | ||
| uses: Gr1N/setup-poetry@48b0f77c8c1b1b19cb962f0f00dff7b4be8f81ec # v9 | ||
| with: | ||
| poetry-version: "2.2.0" | ||
|
|
||
| - name: Install dependencies | ||
| run: poetry install | ||
|
|
||
| - name: Run slow marker tuner | ||
| run: | | ||
| poetry run python bin/tune_slow_markers.py --timeout 7.0 --remove-slow | ||
|
|
||
| - name: Check for changes | ||
| id: check_changes | ||
| run: | | ||
| if [[ -n $(git status --porcelain) ]]; then | ||
| echo "changes=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "changes=false" >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
||
| - name: Commit and push changes | ||
| if: steps.check_changes.outputs.changes == 'true' | ||
| run: | | ||
| git config --local user.email "github-actions[bot]@users.noreply.github.com" | ||
| git config --local user.name "github-actions[bot]" | ||
| git add tests/ | ||
| git commit -m "chore: Auto-tune pytest slow markers" | ||
| git push | ||
|
|
||
| - name: Add reaction to comment | ||
| uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5.0.0 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| repository: ${{ github.event.client_payload.github.payload.repository.full_name }} | ||
| comment-id: ${{ github.event.client_payload.github.payload.comment.id }} | ||
| reactions: rocket | ||
|
|
||
| - name: Comment on PR with results | ||
| uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5.0.0 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| issue-number: ${{ github.event.client_payload.github.payload.issue.number }} | ||
| body: | | ||
| ✅ Slow marker tuning complete! | ||
|
|
||
| ${{ steps.check_changes.outputs.changes == 'true' && 'Changes have been committed and pushed to this PR.' || 'No changes were needed - all markers are already correctly set.' }} | ||
|
|
||
| - name: Comment on PR with failure | ||
| if: failure() | ||
| uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5.0.0 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| issue-number: ${{ github.event.client_payload.github.payload.issue.number }} | ||
| body: | | ||
| ❌ Slow marker tuning failed. Please check the [workflow logs](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) for details. |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 24 days ago
To address the flagged issue, we should explicitly set the permissions block within the workflow (applied at the root or job level). The workflow requires the ability to commit/push changes to the repository (which uses contents: write) and interact with pull request comments (pull-requests: write, and possibly issues: write if working with issue comments). The safest and minimal permission set will be at least contents: write and pull-requests: write. If comments are only on pull requests, pull-requests: write suffices; if comments might be on issues, add issues: write. The explicit permissions block should be added immediately below name: Tune Slow Markers Command for all jobs, or within the tune-slow-markers job.
| @@ -1,9 +1,9 @@ | ||
| name: Tune Slow Markers Command | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| issues: write | ||
|
|
||
| on: | ||
| repository_dispatch: | ||
| types: [tune-slow-markers-command] | ||
|
|
||
| env: | ||
| AIRBYTE_ANALYTICS_ID: ${{ vars.AIRBYTE_ANALYTICS_ID }} | ||
|
|
📝 WalkthroughWalkthroughThis PR introduces an automated pytest slow marker tuning feature. It adds a GitHub Actions workflow triggered via repository dispatch that runs a new Python script to analyze test execution times, automatically adding or removing @pytest.mark.slow decorators based on a configurable threshold, then commits the changes back to the PR. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dispatch as Slash Command<br/>Dispatch
participant Workflow as tune-slow-markers<br/>Workflow
participant Script as tune_slow_markers.py
participant Pytest
participant FileSystem as File System
participant PR as PR Comment
User->>Dispatch: Trigger command
Dispatch->>Workflow: Dispatch event
Workflow->>Workflow: Checkout code
Workflow->>Script: Run with timeout & --remove-slow
Script->>Pytest: Run pytest (collection mode)
Pytest-->>Script: Return test timings
Script->>Script: Analyze tests vs threshold
Script->>FileSystem: Add/remove @pytest.mark.slow<br/>(AST-based modifications)
alt Changes detected
Workflow->>FileSystem: Commit & push changes
Workflow->>PR: Post success summary
else No changes
Workflow->>PR: Post no-changes summary
end
Workflow->>PR: Add reaction to trigger comment
opt On failure
Workflow->>PR: Post failure message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The change involves three distinct areas: two GitHub Actions workflow files (straightforward additions), a new Python utility script with moderate logic density (AST manipulation, file I/O, test result parsing), and a configuration entry. The script contains error handling and multiple helper functions that warrant careful review, though the patterns are relatively consistent. The workflow files are more templated in nature. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
bin/tune_slow_markers.py (5)
71-71: Consider using a set literal here?The linter suggests using a set literal for membership testing. Would you like to update this to:
- if result.returncode not in (0, 5): + if result.returncode not in {0, 5}:This is a minor performance optimization that makes the membership test slightly faster, wdyt?
153-175: Simplify nested if statements?The linter suggests combining the nested if statements for better readability. Would you like to flatten these conditions? For example:
def has_slow_marker(test_node: ast.FunctionDef) -> bool: """Check if a test function has @pytest.mark.slow decorator.""" for decorator in test_node.decorator_list: if isinstance(decorator, ast.Attribute): - if ( - isinstance(decorator.value, ast.Attribute) - and isinstance(decorator.value.value, ast.Name) - and decorator.value.value.id == "pytest" - and decorator.value.attr == "mark" - and decorator.attr == "slow" - ): + if (isinstance(decorator.value, ast.Attribute) and + isinstance(decorator.value.value, ast.Name) and + decorator.value.value.id == "pytest" and + decorator.value.attr == "mark" and + decorator.attr == "slow"): return True elif isinstance(decorator, ast.Call): - if isinstance(decorator.func, ast.Attribute): - if ( - isinstance(decorator.func.value, ast.Attribute) - and isinstance(decorator.func.value.value, ast.Name) - and decorator.func.value.value.id == "pytest" - and decorator.func.value.attr == "mark" - and decorator.func.attr == "slow" - ): - return True + if (isinstance(decorator.func, ast.Attribute) and + isinstance(decorator.func.value, ast.Attribute) and + isinstance(decorator.func.value.value, ast.Name) and + decorator.func.value.value.id == "pytest" and + decorator.func.value.attr == "mark" and + decorator.func.attr == "slow"): + return True return FalseThis addresses the linting issues while maintaining the same logic, wdyt?
178-224: Address linting issues in add_slow_marker_to_file?The linter flagged a few issues here:
- Boolean positional argument (line 178): Consider making
dry_runkeyword-only to improve clarity at call sites:-def add_slow_marker_to_file(file_path: str, test_functions: set[str], dry_run: bool = False) -> int: +def add_slow_marker_to_file(file_path: str, test_functions: set[str], *, dry_run: bool = False) -> int:
- Nested if statement (line 202): Could combine into a single condition:
- if isinstance(node, ast.FunctionDef) and node.name in test_functions: - if not has_slow_marker(node): + if isinstance(node, ast.FunctionDef) and node.name in test_functions and not has_slow_marker(node):
- Line too long (line 213): Could wrap the print statement:
- print( - f" [DRY RUN] Would add @pytest.mark.slow to {node.name} at line {lineno + 1}" - ) + print(f" [DRY RUN] Would add @pytest.mark.slow to {node.name} " + f"at line {lineno + 1}")These are minor style improvements. The logic itself looks solid! Wdyt?
227-275: Simplify remove_slow_marker_from_file and address linting issues?Several linting issues here:
- Boolean positional argument (line 228): Same as above, consider keyword-only:
-def remove_slow_marker_from_file( - file_path: str, test_functions: set[str], dry_run: bool = False -) -> int: +def remove_slow_marker_from_file( + file_path: str, test_functions: set[str], *, dry_run: bool = False +) -> int:
- Too many nested blocks (line 252): The function has 6 levels of nesting (limit is 5). Could you extract the marker-removal logic into a helper? Something like:
def _find_and_remove_marker(node, lines, test_functions, dry_run): """Helper to find and remove slow marker from a test function.""" if node.name not in test_functions or not has_slow_marker(node): return None for i in range(max(0, node.lineno - 10), node.lineno): if lines[i].strip() == "@pytest.mark.slow": if dry_run: print(f" [DRY RUN] Would remove @pytest.mark.slow from {node.name} " f"at line {i + 1}") else: print(f" Removing @pytest.mark.slow from {node.name} at line {i + 1}") return i return NoneThen use it in the main loop:
for node in ast.walk(tree): if isinstance(node, ast.FunctionDef): lineno = _find_and_remove_marker(node, lines, test_functions, dry_run) if lineno is not None: lines_to_remove.append(lineno) markers_removed += 1This would reduce nesting and improve readability. Wdyt?
278-353: Consider extracting helpers to reduce main function size?The
mainfunction has 53 statements (limit is 50). It's doing quite a bit! Could you extract some sections into helper functions? For example:def _process_slow_marker_additions( slow_tests: set[tuple[str, str]], dry_run: bool ) -> int: """Add slow markers to tests that exceed the threshold.""" tests_by_file: dict[str, set[str]] = {} for file_path, test_func in slow_tests: if file_path not in tests_by_file: tests_by_file[file_path] = set() tests_by_file[file_path].add(test_func) total_added = 0 for file_path, test_funcs in tests_by_file.items(): print(f"Processing {file_path}...") added = add_slow_marker_to_file(file_path, test_funcs, dry_run=dry_run) total_added += added return total_added def _process_slow_marker_removals( fast_tests: set[tuple[str, str]], dry_run: bool ) -> int: """Remove slow markers from tests that finish within the threshold.""" # Similar logic...This would make the main function more readable and address the linting warning. Wdyt?
.github/workflows/tune-slow-markers-command.yml (1)
35-37: Consider adding a timeout to the tuner step?Since this step runs the entire test suite to collect timing data, it could potentially take a long time or hang if tests have issues. Would you like to add a
timeout-minutesto this step as a safety measure? Something like:- name: Run slow marker tuner + timeout-minutes: 60 run: | poetry run python bin/tune_slow_markers.py --timeout 7.0 --remove-slowThis would prevent the workflow from running indefinitely if something goes wrong. The exact timeout value would depend on how long your full test suite typically takes. Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/slash_command_dispatch.yml(1 hunks).github/workflows/tune-slow-markers-command.yml(1 hunks)bin/__init__.py(1 hunks)bin/tune_slow_markers.py(1 hunks)pyproject.toml(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Linters
bin/__init__.py
[error] 1-1: D104 Missing docstring in public package. CPY001 Missing copyright notice at top of file.
bin/tune_slow_markers.py
[error] 21-21: D103 Missing docstring in public function
[error] 71-71: PLR6201 Use a set literal when testing for membership
[error] 106-106: E501 Line too long (101 > 100)
[error] 165-165: SIM102 Use a single if statement instead of nested if statements
[error] 178-178: FBT001 Boolean-typed positional argument in function definition
[error] 178-178: FBT002 Boolean default positional argument in function definition
[error] 202-202: SIM102 Use a single if statement instead of nested if statements
[error] 213-213: E501 Line too long (102 > 100)
[error] 228-228: FBT001 Boolean-typed positional argument in function definition
[error] 228-228: FBT002 Boolean default positional argument in function definition
[error] 252-252: PLR1702 Too many nested blocks (6 > 5)
[error] 253-253: SIM102 Use a single if statement instead of nested if statements
[error] 263-263: E501 Line too long (110 > 100)
[error] 278-278: PLR0915 Too many statements (53 > 50)
🪛 GitHub Check: CodeQL
.github/workflows/tune-slow-markers-command.yml
[warning] 12-82: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (14)
.github/workflows/slash_command_dispatch.yml (1)
37-37: LGTM!The addition of the
tune-slow-markerscommand follows the existing pattern and integrates well with the new workflow.pyproject.toml (3)
6-6: LGTM!Adding the
binpackage is necessary to enable the CLI entry point and follows Poetry's packaging conventions correctly.
135-135: LGTM!The CLI entry point is properly configured and follows the same pattern as the other scripts in the project.
141-141: LGTM!The Poe task is well-defined with a clear help message and follows the existing task pattern in the project.
bin/tune_slow_markers.py (4)
1-11: LGTM!The module header and docstring are well-structured and provide clear usage instructions.
118-150: LGTM!Both
find_slow_testsandfind_fast_testshave clear, symmetric logic and properly handle the threshold comparison. The use of sets for deduplication is appropriate.
356-357: LGTM!The script entry point follows the standard Python pattern correctly.
91-98: Let me gather the necessary information differently. I'll search for pytest's duration output format stability and also examine the actual file context.Now let me examine the actual file to understand the context better and verify the regex pattern used:Now let me examine the actual file in the repository to see the full context:
Based on my investigation, the pytest
--durationsoutput format has remained stable and consistent. The format matches the documented pattern with examples like "0.30s call test_some_are_slow.py::test_funcslow2", which aligns with the regex pattern in the code:r"^([\d.]+)s\s+(?:call|setup|teardown)\s+(.+)$".The output format is well-established and hasn't changed in the pytest 8.x series. No breaking changes to the duration output formatting were documented in the pytest 8.2.0+ release notes. The regex pattern correctly captures the documented format across your specified pytest version range (^8.2.0).
Regex pattern is correct and stable—no changes needed. The code uses the proper format for parsing pytest's duration output, which has been consistent since the feature was introduced. If you wanted to add defensive error handling (e.g., logging unparsed lines), that's a nice-to-have but not necessary given the reliability of the output format, wdyt?
.github/workflows/tune-slow-markers-command.yml (6)
1-9: LGTM!The workflow trigger and environment setup follow the standard GitHub Actions patterns for slash command dispatch.
22-33: LGTM!The Python and Poetry setup steps are properly configured with appropriate versions matching the project requirements.
57-63: LGTM!The reaction step provides good user feedback for the slash command invocation.
65-73: LGTM!The success comment provides clear feedback about whether changes were made, which is helpful for users.
75-82: LGTM!The failure comment provides helpful guidance with a link to the workflow logs for debugging.
48-55: I need to verify whether those external test files actually use pytest markers that would be affected by this workflow.Based on my verification, I found test files that exist outside the
tests/directory:
bin/test_mcp_tool.pyexamples/run_integ_test_source.pyexamples/run_perf_test_reads.pyexamples/run_test_source.pyexamples/run_test_source_single_stream.pyThe original review concern has merit. If any of these external test files contain
@pytest.mark.slowmarkers that need tuning, they would not be captured bygit add tests/. However, I cannot definitively determine the severity without examining the workflow's implementation and verifying whether these files contain slow markers that this workflow is meant to process.Verify whether test files outside tests/ need marker tuning
The workflow currently stages only
tests/withgit add tests/on line 53, but test files exist inbin/andexamples/directories. If those files contain slow pytest markers that should be auto-tuned by this workflow, they'll be missed.Check whether the marker-tuning logic processes these external directories, and if it does, consider:
- git add tests/ + git add .Or add other test directories explicitly if they're in scope. Otherwise, if the workflow intentionally tunes only unit tests in
tests/, you can confirm that's the design. Wdyt?
| tune-slow-markers: | ||
| name: Tune Slow Markers | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout PR branch | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| repository: ${{ github.event.client_payload.pull_request.head.repo.full_name }} | ||
| ref: ${{ github.event.client_payload.pull_request.head.ref }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit permissions block to the workflow?
CodeQL flagged that this workflow doesn't limit the permissions of GITHUB_TOKEN. Since this workflow needs to commit and push changes (line 55), would you consider adding an explicit permissions block at the job level? Something like:
jobs:
tune-slow-markers:
name: Tune Slow Markers
runs-on: ubuntu-latest
+ permissions:
+ contents: write # For git push
+ pull-requests: write # For commenting on PRs
+ issues: write # For reacting to comments
steps:This follows the principle of least privilege and makes the required permissions explicit. Wdyt?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/tune-slow-markers-command.yml around lines 11 to 20 the job
does not declare explicit GITHUB_TOKEN permissions; add a job-level permissions
block granting only the minimum required permission to push commits (e.g.,
permissions: contents: write) directly under the tune-slow-markers job header so
the workflow can commit/push while following least-privilege practices.
bin/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Add missing copyright notice and module docstring to resolve linter failures.
The linter is blocking this PR due to two violations: D104 (missing docstring) and CPY001 (missing copyright). These are quick fixes. Would you like to add a standard copyright notice and a brief docstring describing the bin package as containing CLI utilities? Something like:
+# Copyright (c) 2024 Airbyte, Inc., all rights reserved.
+"""Automation tools and CLI utilities for PyAirbyte."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Copyright (c) 2024 Airbyte, Inc., all rights reserved. | |
| """Automation tools and CLI utilities for PyAirbyte.""" |
🧰 Tools
🪛 GitHub Actions: Run Linters
[error] 1-1: D104 Missing docstring in public package. CPY001 Missing copyright notice at top of file.
🤖 Prompt for AI Agents
In bin/__init__.py around lines 1–1, add a standard copyright header and a
module-level docstring to satisfy CPY001 and D104: insert a short copyright
notice (year and project/owner) at the top of the file followed by a one-line
docstring that explains this package contains CLI/command-line utilities (e.g.,
"CLI utilities for <project>"). Save and re-run the linter to confirm the
violations are resolved.
| def parse_args() -> argparse.Namespace: | ||
| parser = argparse.ArgumentParser( | ||
| description="Automatically tune pytest slow markers based on test execution time" | ||
| ) | ||
| parser.add_argument( | ||
| "--timeout", | ||
| type=float, | ||
| default=7.0, | ||
| help="Timeout threshold in seconds (default: 7.0)", | ||
| ) | ||
| parser.add_argument( | ||
| "--dry-run", | ||
| action="store_true", | ||
| help="Show what would be changed without modifying files", | ||
| ) | ||
| parser.add_argument( | ||
| "--remove-slow", | ||
| action="store_true", | ||
| help="Also remove slow markers from tests that finish within the threshold", | ||
| ) | ||
| parser.add_argument( | ||
| "--test-path", | ||
| type=str, | ||
| default="tests/", | ||
| help="Path to test directory (default: tests/)", | ||
| ) | ||
| return parser.parse_args() |
There was a problem hiding this comment.
Add docstring to parse_args function?
The linter flagged this function as missing a docstring. Would you mind adding a brief docstring to describe what this function does and returns? Something like:
def parse_args() -> argparse.Namespace:
+ """Parse command-line arguments for the slow marker tuner."""
parser = argparse.ArgumentParser(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def parse_args() -> argparse.Namespace: | |
| parser = argparse.ArgumentParser( | |
| description="Automatically tune pytest slow markers based on test execution time" | |
| ) | |
| parser.add_argument( | |
| "--timeout", | |
| type=float, | |
| default=7.0, | |
| help="Timeout threshold in seconds (default: 7.0)", | |
| ) | |
| parser.add_argument( | |
| "--dry-run", | |
| action="store_true", | |
| help="Show what would be changed without modifying files", | |
| ) | |
| parser.add_argument( | |
| "--remove-slow", | |
| action="store_true", | |
| help="Also remove slow markers from tests that finish within the threshold", | |
| ) | |
| parser.add_argument( | |
| "--test-path", | |
| type=str, | |
| default="tests/", | |
| help="Path to test directory (default: tests/)", | |
| ) | |
| return parser.parse_args() | |
| def parse_args() -> argparse.Namespace: | |
| """Parse command-line arguments for the slow marker tuner.""" | |
| parser = argparse.ArgumentParser( | |
| description="Automatically tune pytest slow markers based on test execution time" | |
| ) | |
| parser.add_argument( | |
| "--timeout", | |
| type=float, | |
| default=7.0, | |
| help="Timeout threshold in seconds (default: 7.0)", | |
| ) | |
| parser.add_argument( | |
| "--dry-run", | |
| action="store_true", | |
| help="Show what would be changed without modifying files", | |
| ) | |
| parser.add_argument( | |
| "--remove-slow", | |
| action="store_true", | |
| help="Also remove slow markers from tests that finish within the threshold", | |
| ) | |
| parser.add_argument( | |
| "--test-path", | |
| type=str, | |
| default="tests/", | |
| help="Path to test directory (default: tests/)", | |
| ) | |
| return parser.parse_args() |
🧰 Tools
🪛 GitHub Actions: Run Linters
[error] 21-21: D103 Missing docstring in public function
🤖 Prompt for AI Agents
In bin/tune_slow_markers.py around lines 21 to 47, the parse_args function is
missing a docstring; add a concise docstring immediately under the def line that
briefly describes that the function builds and parses command-line arguments for
tuning pytest slow markers and returns an argparse.Namespace with parsed options
(timeout, dry_run, remove_slow, test_path). Make it one or two short sentences
and mention the return type.
- Convert script to use PEP 723 inline script metadata - Add shebang for direct execution with uv run - Add get_pytest_command() to auto-detect pytest command - Update GitHub workflow to use uv instead of poetry - Remove bin package from pyproject.toml (no longer needed) - Update poe task to use uv run - Delete bin/__init__.py (no longer needed) Co-Authored-By: AJ Steers <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| - name: Check for changes | ||
| id: check_changes | ||
| run: | |
There was a problem hiding this comment.
📝 [actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting [shellcheck]
|
|
||
| - name: Check for changes | ||
| id: check_changes | ||
| run: | |
There was a problem hiding this comment.
📝 [actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:4:27: Double quote to prevent globbing and word splitting [shellcheck]
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/tune-slow-markers-command.yml (2)
38-43: Fix shellcheck quoting warnings?Shellcheck flagged missing quotes around the command substitution. Would you mind adding quotes to prevent potential word splitting?
- if [[ -n $(git status --porcelain) ]]; then + if [[ -n "$(git status --porcelain)" ]]; then echo "changes=true" >> $GITHUB_OUTPUT else echo "changes=false" >> $GITHUB_OUTPUTThis is more defensive, though in practice the output shouldn't contain problematic characters. Wdyt?
11-20: Add explicit permissions block for least privilege?CodeQL and past reviews flagged that this workflow doesn't limit
GITHUB_TOKENpermissions. Since the workflow commits and pushes changes (lines 47-52) and comments on PRs (lines 54-79), would you consider adding explicit permissions at the job level? Something like:jobs: tune-slow-markers: name: Tune Slow Markers runs-on: ubuntu-latest + permissions: + contents: write # For git push + pull-requests: write # For commenting on PRs steps:This follows the principle of least privilege and makes required permissions explicit. Wdyt?
🧹 Nitpick comments (4)
bin/tune_slow_markers.py (4)
28-42: Fix linting issues?The linter flagged a couple of style issues:
- Line 30 has trailing whitespace
- Line 36:
elifis unnecessary after areturnstatementWould you mind cleaning these up?
def get_pytest_command() -> list[str]: """Determine the appropriate pytest command to use. - + Returns: List of command parts to run pytest """ if Path("pyproject.toml").exists() and shutil.which("poetry"): return ["poetry", "run", "pytest"] - elif shutil.which("pytest"): + if shutil.which("pytest"): return ["pytest"] - else: - raise RuntimeError( - "pytest not found. Please ensure pytest is installed " - "or run from a poetry project." - ) + raise RuntimeError( + "pytest not found. Please ensure pytest is installed " + "or run from a poetry project." + )Wdyt?
82-82: Remove trailing whitespace?Line 82 has trailing whitespace flagged by the linter. Quick fix:
pytest_cmd = get_pytest_command() - + cmd = [
185-207: Consider simplifying AST checks to reduce complexity?The linter flagged several style issues here:
- Line 188: Too many boolean expressions (6 > 5)
- Line 196: Unnecessary
elifafterreturn- Lines 196-199: Could use single
ifinstead of nestedThe logic is correct but could be refactored for readability. Would you consider extracting a helper function to check if a decorator matches
pytest.mark.slow? Something like:def _is_pytest_slow_marker(decorator: ast.expr) -> bool: """Check if decorator is pytest.mark.slow or pytest.mark.slow().""" # Check @pytest.mark.slow if isinstance(decorator, ast.Attribute): return _is_pytest_mark_attr(decorator, "slow") # Check @pytest.mark.slow() if isinstance(decorator, ast.Call) and isinstance(decorator.func, ast.Attribute): return _is_pytest_mark_attr(decorator.func, "slow") return False def _is_pytest_mark_attr(node: ast.Attribute, mark_name: str) -> bool: """Check if attribute node is pytest.mark.<mark_name>.""" return ( isinstance(node.value, ast.Attribute) and isinstance(node.value.value, ast.Name) and node.value.value.id == "pytest" and node.value.attr == "mark" and node.attr == mark_name )This would reduce complexity and make the checks more maintainable. Wdyt? Or is this overkill for the current scope?
323-404: Consider extracting helper functions to reduce main() complexity?The linter flagged that
main()has 53 statements (limit is 50). The logic is clear, but you might want to extract a few helper functions for better maintainability? For example:
_process_slow_markers(slow_tests, dry_run)for lines 352-365_process_fast_markers(fast_tests, dry_run)for lines 375-390This would make
main()more readable and easier to test individual pieces. But given this is a single-purpose script, the current structure is fine too. Wdyt? Just flagging in case you want to address the linter warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tune-slow-markers-command.yml(1 hunks)bin/tune_slow_markers.py(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
bin/tune_slow_markers.py (1)
airbyte/_util/meta.py (1)
which(179-190)
🪛 GitHub Actions: Run Linters
bin/tune_slow_markers.py
[error] 1-1: EXE003 Shebang should contain python
[error] 30-30: W293 Blank line contains whitespace
[error] 36-36: RET505 Unnecessary elif after return statement
[error] 82-82: W293 Blank line contains whitespace
[error] 188-188: PLR0916 Too many Boolean expressions (6 > 5)
[error] 196-196: RET505 Unnecessary elif after return statement
[error] 196-199: SIM102 Use a single if statement instead of nested if statements
[error] 323-323: PLR0915 Too many statements (53 > 50)
🪛 GitHub Check: CodeQL
.github/workflows/tune-slow-markers-command.yml
[warning] 12-79: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (11)
.github/workflows/tune-slow-markers-command.yml (2)
45-52: LGTM! Git operations are appropriately scoped.The commit step correctly:
- Only runs when changes are detected
- Uses the github-actions bot identity
- Stages only the
tests/directory (preventing accidental commits of other changes)- Includes a clear conventional commit message
54-79: Verify client_payload structure matches slash command dispatch?The PR commenting steps reference several nested paths in
client_payload:
- Line 58:
github.payload.repository.full_name- Line 59:
github.payload.comment.id- Line 66:
github.payload.issue.numberCould you verify these paths match what the slash command dispatch action actually provides? The structure looks correct for peter-murray/slash-command-dispatch, but it would be good to confirm this works end-to-end, especially since the author noted limited testing. Wdyt about adding this to your testing checklist?
bin/tune_slow_markers.py (9)
1-7: Shebang is correct for uv PEP 723 scripts.The linter flagged the shebang, but
#!/usr/bin/env -S uv runis the correct format for PEP 723 inline script metadata (which you're using on lines 2-7). This allows the script to be executed directly with its dependencies managed by uv. The linter warning can be safely ignored here. 👍
45-72: LGTM! Argument parsing is well-structured.The CLI arguments are clearly defined with sensible defaults. The docstring addresses the previous review comment. Nice work on the help text for each option! 👍
114-121: Verify pytest durations output format?The regex
r"^([\d.]+)s\s+(?:call|setup|teardown)\s+(.+)$"parses pytest's durations output. A couple of questions:
- Does this handle all pytest output formats across different versions? The pattern assumes a specific format.
- Should timings be aggregated per test? Currently, a test with slow setup + fast call + fast teardown would create three separate entries. If setup takes 8s but call takes 1s, would you want to mark it slow based on setup time?
The author mentioned this hasn't been fully tested. Would you want to verify this on a real test suite to catch edge cases? Wdyt?
126-140: LGTM! Test name parsing handles common pytest formats.The function correctly handles:
- Simple test names:
file.py::test_func- Class methods:
file.py::TestClass::test_method(usesparts[-1])- Parametrized tests: strips the
[params]suffixThe docstring was also fixed to address the previous line-length concern. Nice!
143-182: LGTM! Test filtering logic is correct.Both functions properly identify tests based on the threshold. One small observation:
find_slow_testsprints each test found (lines 158-161), butfind_fast_testsis silent. Would you want to add similar logging tofind_fast_testsfor consistency, or is the asymmetry intentional to reduce noise? Either way works, just checking! 😊
210-262: Consider decorator insertion order?The marker is inserted at
node.lineno - 1, which places it above all existing decorators. In practice, this means if a test has:@pytest.mark.integration def test_foo(): ...It becomes:
@pytest.mark.slow @pytest.mark.integration def test_foo(): ...Is this the desired order, or would you prefer
@pytest.mark.slowto be closer to the function def (after other markers)? The current approach is simpler and probably fine, just wanted to check if you have a preference! Wdyt?
265-320: LGTM! Marker removal logic is sound.The function correctly:
- Searches up to 10 lines before the function def (reasonable limit)
- Uses exact match to avoid false positives
- Deletes in reverse order to maintain line numbers
- Stops after first match per function
The 10-line search window should cover typical decorator stacks. If you encounter edge cases where decorators are further away, you can always adjust this later. 👍
50-54: Validate the 7.0s default threshold with real test data?The default timeout is set to 7.0 seconds here (and in the workflow). Since you mentioned limited testing, would you want to:
- Run this on your actual test suite to see the distribution of test times?
- Verify that 7.0s is a good cutoff that doesn't mark too many/too few tests as slow?
A quick histogram of test durations would help validate this threshold is appropriate for your project. Want me to help generate a script to analyze test durations without modifying anything? Wdyt?
1-404: Test the script end-to-end before merging?Given your note that this was "only dry-run tested on a single file," would you want to run a more comprehensive test before merging? Suggested test plan:
- Run with
--dry-runon the full test suite to see what would change- Test on a small subset without
--dry-runand verify the AST modifications preserve formatting- Run pytest after modifications to ensure tests still work
- Test the
--remove-slowflag to ensure it doesn't remove markers incorrectly- Verify the GitHub Actions workflow works end-to-end with a test PR
This is a powerful tool that modifies code automatically, so thorough testing would give confidence it won't cause issues. I can help generate verification scripts if useful! 😊
feat: Add automated pytest slow marker tuner
Summary
This PR adds an automated tool for tuning pytest
@pytest.mark.slowdecorators based on actual test execution time. The tool runs all tests, measures their duration, and automatically adds or removes slow markers based on a configurable threshold (default: 7.0s).Key components:
bin/tune_slow_markers.py): Standalone Python script that can be run locally or via pipx/tune-slow-markers): GitHub Actions workflow that runs the tuner on PRs and commits changesFeatures:
Review & Testing Checklist for Human
poetry run python bin/tune_slow_markers.py --test-path tests/unit_tests/ --dry-runto see what it would change, then run without--dry-runon a small subset to verify it works correctlyhas_slow_marker()and insertion inadd_slow_marker_to_file()uses AST walking. Test with edge cases like tests with multiple decorators, nested test classes, etc./tune-slow-markerscommand on a test PR to verify it runs correctly and commits changesTest Plan
poetry installpoetry run python bin/tune_slow_markers.py --test-path tests/unit_tests/ --dry-runpoetry run python bin/tune_slow_markers.py --test-path tests/unit_tests/test_progress.pypoetry run poe tune-slow-markers --helpNotes
.github/workflows/python_pytest.yml)@pytest.mark.super_slowtests when collecting timing datapathlib.Path.read_text()andwrite_text()as per project conventionsRequested by: AJ Steers (@aaronsteers)
Devin session: https://app.devin.ai/sessions/b2bd2fdac9314cf18270c81f81305c55
Summary by CodeRabbit