Make E2E tests more comprehensive and less flaky#58
Make E2E tests more comprehensive and less flaky#58jovnc wants to merge 3 commits intogit-mastery:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the E2E test suite by making test prerequisites explicit per test (reducing hidden inter-test dependencies) and by asserting real filesystem/config side effects instead of relying only on CLI output.
Changes:
- Refactored E2E fixtures to use function-scoped setup/download/verify fixtures for clearer, isolated test setup.
- Strengthened E2E assertions to validate generated config/progress files and their contents.
- Updated check/setup/download/verify/progress tests to rely on the new fixtures and more specific output/side-effect expectations.
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 |
|---|---|
| tests/e2e/conftest.py | Replaces the session-scoped exercises fixture with explicit function-scoped setup/download/verify fixtures. |
| tests/e2e/commands/test_setup.py | Adds assertions for default config flags and empty progress.json. |
| tests/e2e/commands/test_download.py | Validates .gitmastery-exercise.json includes the expected exercise_name. |
| tests/e2e/commands/test_verify.py | Verifies verify writes a progress entry with expected fields. |
| tests/e2e/commands/test_progress.py | Asserts progress_remote toggling and that reset removes the exercise entry. |
| tests/e2e/commands/test_check.py | Tightens output assertions to the final “installed and configured” messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jovnc
force-pushed
the
test/improve-e2e-test
branch
from
77814c1 to
2f67a71
Compare
SAN-MUYUN
left a comment
SAN-MUYUN
left a comment
There was a problem hiding this comment.
LGTM! Just need to take note of the test_progress_reset in terms of how we want to test reset of answer.txt.
|
|
||
| log_file = exercises_dir / ".gitmastery.log" | ||
| assert log_file.is_file(), f"Expected {log_file} to exist" | ||
| assert (gitmastery_root / ".gitmastery.log").is_file() |
| progress_json = downloaded_exercise_dir.parent / "progress" / "progress.json" | ||
| entries = json.loads(progress_json.read_text()) | ||
| assert len(entries) == 1 | ||
| entry = entries[0] |
There was a problem hiding this comment.
when running gitmastery verify, new content is added to the end of the list of entries. Since we are asserting that the len(entries) == 1, it should be fine in this case.
| res = runner.run(["check", "github"]) | ||
| res.assert_success() | ||
| res.assert_stdout_contains("Github CLI is installed") | ||
| res.assert_stdout_contains("Github CLI is installed and configured") |
| """Test the download command output successfully performs the download for hands-on.""" | ||
| res = runner.run(["download", HANDS_ON_NAME], cwd=exercises_dir) | ||
| def test_download_hands_on(runner: BinaryRunner, gitmastery_root: Path) -> None: | ||
| """download creates the hands-on folder.""" |
There was a problem hiding this comment.
I like this change in comment, it describest the test case more accurately.
| res.assert_success() | ||
| progress_json = verified_exercise_dir.parent / "progress" / "progress.json" | ||
| # TODO: need to verify that the exercise itself progress was reset, not just progress.json was cleared | ||
| assert json.loads(progress_json.read_text()) == [] |
There was a problem hiding this comment.
This is a good starting point. For future implementation, we can try to test an exercise that requires reset of answer.txt.
Changes Made
Refactored conftest.py to replace the single session-scoped exercises_dir fixture with three function-scoped fixtures (gitmastery_root, downloaded_exercise_dir, verified_exercise_dir), making each test's prerequisites explicit and eliminating hidden ordering dependencies
Strengthened assertions in existing tests to validate actual file and state side effects, not just CLI output: