test: reduce min_sample_size from 30 to 10 in integration tests#38
test: reduce min_sample_size from 30 to 10 in integration tests#38
Conversation
Rewrite test_integration.py with a proper fixture-based structure: - session-scoped github_env and date_windows fixtures for shared setup - module-scoped fetched_workspace fixture that runs fetch once and reuses the database across all classify tests - date windows computed dynamically relative to today rather than hardcoded values - four parametrised fetch variants (default, with-dates, reset-db, config) - dedicated classify tests: table output, stricter threshold, JSON output, and --exclude-primary-merged Expand the README Development section with instructions for running unit tests only, integration tests only, and each individual integration test by its pytest node ID. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Lower the minimum sample threshold across all integration tests so they can pass against repositories with fewer than 30 merged PRs in the collation window. - tests/test_integration.py: add --min-samples 10 to all classify commands that did not already specify it; update the exclude-primary-merged test from 5 to 10 for consistency - tests/test_outlier_detection_integration.py: change all explicit min_sample_size=30 calls to min_sample_size=10; update the InsufficientDataError match string accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66651535ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "--min-samples", | ||
| "10", |
There was a problem hiding this comment.
Increasing this test's --min-samples to 10 makes the --exclude-primary-merged scenario fail whenever the live fixture repo has only 5–9 non-primary-branch merges in the analysis window, because outlier detection exits with InsufficientDataError when len(prs) < min_sample_size (src/review_classification/analysis/outlier_detector.py, calculate_repository_statistics). Since this test targets real GitHub data, that tighter threshold reintroduces the same availability/flakiness problem this change is trying to reduce.
Useful? React with 👍 / 👎.
1 participant
What
Reduces
min_sample_sizefrom 30 to 10 in all integration tests.Why
A threshold of 30 means the classify tests fail against any repository
that has fewer than 30 PRs merged in the collation window. Lowering to 10
makes the tests more resilient without compromising their ability to detect
outliers — statistical Z-score analysis is still meaningful at this sample
size for the metrics being tested.
Changes
tests/test_integration.py--min-samples 10totest_classify_example_table_output,test_classify_example_stricter_threshold, andtest_classify_example_json_output(previously using the application default of 30)
test_classify_example_exclude_primary_mergedfrom5to10for consistency across all classify tests
tests/test_outlier_detection_integration.pymin_sample_size=30calls tomin_sample_size=10InsufficientDataErrormatch string from"at least 30"to"at least 10"Testing
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com