Improve Github test ergonomics - #520
Conversation
ivanklee86
commented
Jan 19, 2026
- Add CLAUDE.md
- Experiment with using Claude to improve test ergonomics.
…n't want to mess with Github.
There was a problem hiding this comment.
Pull request overview
This pull request refactors GitHub integration tests to improve ergonomics by introducing a dedicated github_token fixture that validates token availability and automatically skips tests when the token is not set. A comprehensive CLAUDE.md development guidelines document is also added to help contributors understand the project's coding standards and testing patterns.
Changes:
- Introduced
github_tokenfixture that validates token availability and skips tests gracefully - Refactored test files to use the new
github_tokenfixture instead of directly accessing environment variables - Added comprehensive CLAUDE.md documentation covering coding style, testing patterns, and development workflows
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures.py | Added github_token fixture with proper validation and test skipping; updated github_pr_clean to depend on it |
| tests/unit/reporters/test_gh.py | Updated test functions to use github_token fixture parameter instead of os.getenv() calls; removed unused os import |
| tests/integration/test_cli.py | Updated CLI test to use github_token fixture parameter; removed unused os import |
| CLAUDE.md | Added comprehensive development guidelines document covering coding standards, testing patterns, and workflows |
| .devcontainer/post_install.sh | Added Claude installation step to devcontainer setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from dotenv import load_dotenv | ||
|
|
||
| @pytest.fixture | ||
| def secrets(): | ||
| """Load environment variables for tests requiring credentials.""" | ||
| load_dotenv() | ||
| yield | ||
|
|
||
| @pytest.fixture | ||
| def github_pr_clean(secrets): | ||
| """Provide a clean GitHub PR for testing.""" | ||
| g = Github(os.getenv("RUBRICAL_TEST_GITHUB_ACCESS_TOKEN")) | ||
| repo = g.get_repo(GITHUB_REPO_NAME) | ||
| pr = repo.get_pull(GITHUB_TEST_PR) | ||
|
|
||
| # Cleanup before test | ||
| for issue_comment in pr.get_issue_comments(): | ||
| issue_comment.delete() | ||
| time.sleep(5) |
There was a problem hiding this comment.
The fixture example in the documentation is outdated and doesn't match the actual implementation. The github_pr_clean fixture now takes github_token as a parameter (not secrets), and should use the token parameter directly instead of calling os.getenv(). This example should be updated to match the new implementation in tests/fixtures.py.
| from dotenv import load_dotenv | |
| @pytest.fixture | |
| def secrets(): | |
| """Load environment variables for tests requiring credentials.""" | |
| load_dotenv() | |
| yield | |
| @pytest.fixture | |
| def github_pr_clean(secrets): | |
| """Provide a clean GitHub PR for testing.""" | |
| g = Github(os.getenv("RUBRICAL_TEST_GITHUB_ACCESS_TOKEN")) | |
| repo = g.get_repo(GITHUB_REPO_NAME) | |
| pr = repo.get_pull(GITHUB_TEST_PR) | |
| # Cleanup before test | |
| for issue_comment in pr.get_issue_comments(): | |
| issue_comment.delete() | |
| time.sleep(5) | |
| from github import Github | |
| @pytest.fixture | |
| def github_pr_clean(github_token): | |
| """Provide a clean GitHub PR for testing.""" | |
| g = Github(github_token) | |
| repo = g.get_repo(GITHUB_REPO_NAME) | |
| pr = repo.get_pull(GITHUB_TEST_PR) | |
| # Cleanup before test | |
| for issue_comment in pr.get_issue_comments(): | |
| issue_comment.delete() |
| # Install Claude | ||
| curl -fsSL https://claude.ai/install.sh | bash | ||
|
|
There was a problem hiding this comment.
The URL https://claude.ai/install.sh appears to be non-existent or incorrect. Claude is an AI assistant by Anthropic and doesn't have a standard shell installation script at this URL. This script will likely fail during the devcontainer setup. Consider removing this installation step or replacing it with the correct installation method for the intended tool.
| # Install Claude | |
| curl -fsSL https://claude.ai/install.sh | bash |