test: expand script coverage and add dev setup bootstrap#36
Merged
Conversation
📝 WalkthroughWalkthroughAdds multi-language tests and fixtures, centralizes path/IO utilities (Bash + Python), prefers stats-policy.json with YAML fallback, expands Makefile/CI to run script tests and coverage, introduces a dev environment bootstrap script, and updates many scripts to use the shared utilities and JSON policy loader. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/parity-test/main_test.go`:
- Around line 245-267: The handler in TestRunScenario_DefaultMethodIsGET calls
t.Fatalf from the httptest handler goroutine which is invalid; change the test
to capture the request method via a channel (e.g., create a methodCh before
creating httptest.NewServer, have the handler send r.Method into methodCh
instead of calling t.Fatalf), then in the main test goroutine receive from
methodCh (with a select + timeout or using t.Fatal on timeout) and assert that
the received method equals http.MethodGet; update the httptest handler used in
TestRunScenario_DefaultMethodIsGET and keep runScenario call the same.
In `@scripts/validate-result-schemas.py`:
- Around line 74-81: The sort key for validation errors currently uses
key=lambda err: list(err.path), which can mix ints and strs and raise TypeError;
update the sorting to convert each path element to string (e.g., key=lambda err:
[str(p) for p in err.path]) so comparisons are consistent, keeping the rest of
the logic (Draft202012Validator, validator.iter_errors, errors, first.path,
path_suffix) unchanged.
🧹 Nitpick comments (5)
tests/fixtures/README.md (1)
7-12: Add language specifier to fenced code block.The directory structure block should have a language identifier for consistent markdown linting.
📝 Suggested fix
-``` +```text tests/fixtures/ ├── raw/ # Raw benchmark result JSON files ├── summary/ # Expected summary.json outputs └── compose/ # Docker compose test files</details> </blockquote></details> <details> <summary>scripts/lib.sh (1)</summary><blockquote> `1-2`: **Consider adding strict mode for safety if sourced after variable expansion.** While library files sourced early typically inherit strict mode from the parent script, adding `set -euo pipefail` here would provide defense-in-depth if this file is ever sourced after errors have already occurred or if sourced in isolation during testing. <details> <summary>♻️ Optional: Add strict mode</summary> ```diff #!/usr/bin/env bash +set -euo pipefail resolve_path() {tests/integration/test_run_scripts.bats (1)
13-21: Consider adding output assertions for more robust test validation.The tests verify exit status but don't validate error messages. Adding output checks would catch regressions in error messaging.
♻️ Optional: Add output assertions
`@test` "run-single rejects unknown framework" { run bash scripts/run-single.sh unknown-framework [ "$status" -ne 0 ] + [[ "$output" == *"Unknown framework"* ]] } `@test` "run-all rejects raw dir outside results/latest" { run env RESULTS_RAW_DIR="/tmp" bash scripts/run-all.sh [ "$status" -ne 0 ] + [[ "$output" == *"must"* ]] # Validates path rejection message }scripts/benchlib/io_utils.py (1)
15-28: Consider friendlier JSON errors for CLI callers.
json.loads(...)will surface stack traces on missing/malformed files. Wrapping with SystemExit keeps CLI output consistent.🔧 Possible refinement
-def read_json(path: Path): - return json.loads(path.read_text(encoding="utf-8")) +def read_json(path: Path): + try: + return json.loads(path.read_text(encoding="utf-8")) + except FileNotFoundError as exc: + raise SystemExit(f"JSON file not found: {path}") from exc + except json.JSONDecodeError as exc: + raise SystemExit( + f"Malformed JSON in {path}: {exc.msg} at line {exc.lineno}, column {exc.colno}" + ) from exc @@ def load_json_policy(path: Path, default_on_missing=None): if not path.exists(): if default_on_missing is not None: return default_on_missing raise SystemExit(f"Policy file not found: {path}") - return json.loads(path.read_text(encoding="utf-8")) + return read_json(path)cmd/parity-test/main_test.go (1)
280-316: Make negative contract cases assert the shared validation logic.
Right now the test re-checks field values directly, so it will pass even if the fixture contract checks change. Consider extracting the contract validation into a helper and asserting the helper rejects these cases.♻️ Suggested change for this test
- if tc.scenario.Request.Path != "" && tc.scenario.Response.Status > 0 { - t.Fatalf("expected invalid fixture contract for case %q", tc.name) - } + if isValidScenarioContract(tc.scenario) { + t.Fatalf("expected invalid fixture contract for case %q", tc.name) + }Example helper to add near
TestParityFixtures_AreWellFormedand reuse there too:func isValidScenarioContract(s Scenario) bool { if s.Name == "" { return false } if s.Request.Path == "" { return false } if s.Response.Status <= 0 { return false } return true }
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type
feat- New capabilityfix- Bug fixdocs- Documentation onlytest- Test changeschore- Tooling, CI, maintenanceSummary
This PR expands parity and benchmark-script test coverage, refactors duplicated script helper logic, and improves local/CI setup ergonomics with a bootstrap script and Make wrappers. It also updates CI to use Make-based setup instead of inline install scripts and removes actionlint from CI per request.
Changes
cmd/parity-test/main_test.gofor:seedTargetdisabled/missing/non-2xx pathsrunScenariostatus/header/invalid-JSON/default-method pathstests/fixtures/.tests/unit/andtests/integration/.pytest.ini,.coveragerc,.bats-versiontest-go,test-python,test-shell,test-scripts,test-coverage-pythonscripts/lib.shscripts/benchlib/io_utils.pyjsonschemainscripts/validate-result-schemas.py.stats-policy.jsonand JSON-first loading with YAML fallback compatibility.scripts/setup-dev-env.shwith:--cimode--subsetinstall groupssetup-dev-env,setup-dev-env-ci,setup-dev-env-ci-scriptsFollow-up fixes from review comments
t.Fatalfusage inTestRunScenario_DefaultMethodIsGETby asserting request method on test goroutine via channel.err.pathparts.scripts/setup-dev-env.shinstall helpers so failedbrew/aptinstalls return cleanly underset -einstead of aborting unexpectedly.Validation
Checklist
Resolves
Resolves #
Summary by CodeRabbit
New Features
Documentation
Chores