Skip to content

Conversation

@rbx
Copy link
Member

@rbx rbx commented Jan 20, 2026

  • adds anonymized cluster history file for use with tests that need it.
  • add script to run all existing tests. can be replaced with pytest when/if files have the appropriate structure.
  • add github workflow that runs all the tests.

@coderabbitai
Copy link

coderabbitai bot commented Jan 20, 2026

📝 Walkthrough

Walkthrough

Introduces 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

Cohort / File(s) Summary
CI Configuration
.github/workflows/tests.yml
New GitHub Actions workflow that triggers on push/PR to master, runs matrix of Python versions (3.10, 3.11, 3.12), installs dependencies, and executes a sequence of test modules including environment checks, sanity tests, workload/price tests, and sampler tests.
Documentation
CLAUDE.md
Updates test execution guidance with new "Run All Tests" section referencing test/run_all.py, adds comprehensive "Run Individual Tests" subsection with specific test invocations, and documents automated CI testing via GitHub Actions.
Test Orchestration
test/run_all.py
New Python script that sequentially executes predefined test suite, collects per-test results, logs PASS/FAIL outcomes, and provides final summary with exit status 1 on any failure or 0 on all passes.
Test Updates
test/test_env.py, test/test_sampler_hourly.py
Align tests with API changes: test_env.py switches environment to empty render_mode and False quick_plot, updates reset/step calls to handle new return tuple formats, and suppresses verbose output. test_sampler_hourly.py adds RNG parameter to hourly_sampler.sample() calls with NumPy random generator initialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add CI tests' accurately summarizes the main change: adding continuous integration test infrastructure and workflows.
Description check ✅ Passed The description is directly related to the changeset, detailing the addition of cluster history file, test runner script, and GitHub Actions workflow.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 --seed CLI 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: call test/run_all.py to 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.py
test/run_all.py (1)

7-20: Prefer sys.executable over 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"],
 ]

@rbx rbx merged commit 41a3dce into master Jan 21, 2026
4 checks passed
@rbx rbx deleted the add-tests branch January 21, 2026 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants