-
Notifications
You must be signed in to change notification settings - Fork 1
Add CI tests #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CI tests #8
Conversation
📝 WalkthroughWalkthroughIntroduces automated test infrastructure via a new GitHub Actions workflow and test orchestration script (test/run_all.py). Updates documentation and existing test files to align with new test execution patterns, environment initialization approaches, and API changes including RNG parameter support for hourly sampling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CLAUDE.md`:
- Around line 127-129: The docs line referencing “master/main” is inconsistent
with the CI workflow which only triggers on `master`; update either the
documentation or the workflow: either change the CLAUDE.md text to mention only
`master` (replace “master/main” with `master`) or modify the workflow trigger in
`.github/workflows/tests.yml` to include `main` (e.g., add `main` alongside
`master`) so the documentation and the `tests.yml` trigger stay in sync.
🧹 Nitpick comments (3)
test/test_sampler_hourly.py (1)
5-12: Add an optional RNG seed for reproducible test runs.Right now the RNG is unseeded, so outputs vary per run. Consider adding a
--seedCLI option and initializing the RNG from it for reproducibility (especially in CI).♻️ Proposed change
-def main(): - rng = np.random.default_rng() - parser = argparse.ArgumentParser(description='Test the hourly job sampler with Slurm logs.') +def main(): + parser = argparse.ArgumentParser(description='Test the hourly job sampler with Slurm logs.') parser.add_argument('--file-path', required=True, help='Path to the Slurm log file') parser.add_argument('--print-stats', action='store_true', help='Print summary statistics.') parser.add_argument('--test-samples', type=int, default=0, help='Number of hours to sample (default: 0)') parser.add_argument('--test-day', action='store_true', help='Sample a full 24-hour day') + parser.add_argument('--seed', type=int, default=None, help='Seed for RNG (default: random)') args = parser.parse_args() + rng = np.random.default_rng(args.seed).github/workflows/tests.yml (1)
32-69: Optional: calltest/run_all.pyto keep CI and local runs in sync.This avoids duplication and the risk of CI drifting from the canonical test list in
test/run_all.py.♻️ Suggested change
- - name: Run environment check - run: python -m test.test_checkenv - - - name: Run environment test - run: python -m test.test_env - - - name: Run workload generator sanity tests - run: python -m test.test_sanity_workloadgen - - - name: Run workload generator determinism tests - run: python -m test.test_determinism_workloadgen - - - name: Run price history tests - run: python -m test.test_price_history - - - name: Run price cycling tests - run: python -m test.test_prices_cycling - - - name: Run environment sanity tests (quick invariants) - run: python -m test.test_sanity_env --steps 200 - - - name: Run environment sanity tests (full) - run: python -m test.test_sanity_env --check-gym --check-determinism --steps 300 - - - name: Run environment sanity tests (with external data) - run: python -m test.test_sanity_env --prices data/prices_2023.csv --hourly-jobs data/allusers-gpu-30.log --steps 300 - - - name: Run duration sampler test - run: python -m test.test_sampler_duration --print-stats --test-samples 10 - - - name: Run hourly sampler test - run: python -m test.test_sampler_hourly --file-path data/allusers-gpu-30.log --test-day - - - name: Run jobs sampler test - run: python -m test.test_sampler_jobs --file-path data/allusers-gpu-30.log - - - name: Run jobs aggregated sampler test - run: python -m test.test_sampler_jobs_aggregated --file-path data/allusers-gpu-30.log + - name: Run all tests + run: python test/run_all.pytest/run_all.py (1)
7-20: Prefersys.executableover hardcoded"python"for the test runner.This ensures the tests run under the active venv/interpreter across platforms.
♻️ Proposed change
-TESTS = [ - ["python", "-m", "test.test_checkenv"], +PYTHON = sys.executable +TESTS = [ + [PYTHON, "-m", "test.test_checkenv"], ["python", "-m", "test.test_env"], ["python", "-m", "test.test_sanity_workloadgen"], ["python", "-m", "test.test_determinism_workloadgen"], ["python", "-m", "test.test_price_history"], ["python", "-m", "test.test_prices_cycling"], ["python", "-m", "test.test_sanity_env", "--steps", "200"], ["python", "-m", "test.test_sanity_env", "--check-gym", "--check-determinism", "--steps", "300"], ["python", "-m", "test.test_sanity_env", "--prices", "data/prices_2023.csv", "--hourly-jobs", "data/allusers-gpu-30.log", "--steps", "300"], ["python", "-m", "test.test_sampler_duration", "--print-stats", "--test-samples", "10"], ["python", "-m", "test.test_sampler_hourly", "--file-path", "data/allusers-gpu-30.log", "--test-day"], ["python", "-m", "test.test_sampler_jobs", "--file-path", "data/allusers-gpu-30.log"], ["python", "-m", "test.test_sampler_jobs_aggregated", "--file-path", "data/allusers-gpu-30.log"], ]
Uh oh!
There was an error while loading. Please reload this page.