feat(plugin): make performance tests opt-in via --performance-tests - #231
Conversation
- Clarify --categories help to point users toward --performance-tests - Reword --performance-tests help to describe additive behavior accurately - Extract _extra_keep_from_args() helper to deduplicate local/K8s paths - Add TestExtraKeepFromArgs selftests covering both opt-in paths
- Clarify --performance-tests help: note it has no effect when --categories is also specified, removing the silent-precedence UX sharp edge - Add TestPerformanceOptIn K8s path integration tests (test_k8s_default_excludes_performance, test_k8s_flag_removes_exclusion) to prevent local/K8s marker-expr drift
There was a problem hiding this comment.
Pull request overview
This PR makes the performance test category opt-in so vip verify excludes performance tests by default, and users can include them explicitly via a new --performance-tests flag.
Changes:
- Exclude
performancetests from the defaultvip verifymarker expression, with an override via--performance-tests. - Update marker metadata/documentation to indicate
performanceis excluded by default. - Add selftests covering the new CLI behavior (local + K8s command/category assembly).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/pages/getting-started.astro | Updates docs to note performance tests are opt-in via --performance-tests. |
| src/vip/plugin.py | Updates the registered performance marker description to indicate opt-in/default exclusion. |
| src/vip/cli.py | Adds performance to default opt-in exclusions and introduces --performance-tests to re-include it by default. |
| selftests/test_plugin.py | Adds pytester integration tests around performance marker deselection/selection behavior. |
| selftests/test_cli_verify.py | Adds unit tests verifying the CLI marker expression changes (local + K8s paths) and new flag behavior. |
| pyproject.toml | Updates marker listing for performance to indicate opt-in/default exclusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # executed when the user opts in via ``--categories``. These tests check | ||
| # VIP's own configuration rather than the Posit deployment. |
There was a problem hiding this comment.
The comment above _OPT_IN_CATEGORIES says opt-in categories are only executed when the user opts in via --categories, but this PR also introduces --performance-tests as an opt-in mechanism. Please update the comment so it stays accurate (e.g., mention both --categories and dedicated opt-in flags).
| # executed when the user opts in via ``--categories``. These tests check | |
| # VIP's own configuration rather than the Posit deployment. | |
| # executed when the user explicitly opts in, either via ``--categories`` or | |
| # a dedicated opt-in flag (for example ``--performance-tests``). These tests | |
| # check VIP's own configuration rather than the Posit deployment. |
| def test_performance_deselected_by_default(self, selftest_pytester): | ||
| """Performance tests should be deselected when --performance-tests is not passed.""" | ||
| selftest_pytester.makepyfile( | ||
| """ | ||
| import pytest | ||
|
|
||
| @pytest.mark.performance | ||
| def test_load_time(): | ||
| assert True | ||
| """ | ||
| ) | ||
| result = selftest_pytester.runpytest( | ||
| "--vip-config=vip.toml", "-v", "-m", "not config_hygiene and not performance" | ||
| ) |
There was a problem hiding this comment.
This test claims to validate behavior when --performance-tests is not passed, but it explicitly sets the marker expression via -m "not config_hygiene and not performance". Either adjust the test/docstring to reflect that it's testing marker-based deselection, or invoke the VIP CLI path that actually controls the default selection.
| def test_performance_runs_with_flag(self, selftest_pytester): | ||
| """Performance tests should run when --performance-tests equivalent marker is included.""" | ||
| selftest_pytester.makepyfile( | ||
| """ | ||
| import pytest | ||
|
|
||
| @pytest.mark.performance | ||
| def test_load_time(): | ||
| assert True | ||
| """ | ||
| ) | ||
| result = selftest_pytester.runpytest( | ||
| "--vip-config=vip.toml", "-v", "-m", "not config_hygiene" | ||
| ) | ||
| result.assert_outcomes(passed=1) |
There was a problem hiding this comment.
This test name/docstring implies it exercises the --performance-tests flag, but it doesn't pass that flag (it just omits not performance in the -m expression). Consider renaming/rewording it, or change the test to run through the CLI option that this PR adds.
Update the _OPT_IN_CATEGORIES comment to mention --performance-tests as an opt-in mechanism alongside --categories. Replace the two misleading pytester tests (which manually set -m expressions instead of exercising the CLI flag) with unit tests that validate marker-expression assembly through _extra_keep_from_args and _default_marker_expr directly.
# Conflicts: # selftests/test_cli_verify.py
|
Summary
@performance) are now excluded from the defaultvip verifyrun; opt in with the new--performance-testsflag._extra_keep_from_args()helper.--categoriesis also specified (noted in the help text).plugin.pyandpyproject.toml; opt-in status noted on the website.categories=propagation.Closes #211.
Test plan
uv run ruff check src/ src/vip_tests/ selftests/ examples/uv run ruff format --check src/ src/vip_tests/ selftests/ examples/uv run pytest selftests/ -v -k "performance or marker"(27 passed)vip verify --connect-url ... -- --collect-onlyshows 0 perf tests by default, >0 with--performance-tests