ci: test-e2e reworked to use TestPypi#105
Conversation
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes the matrix-driven workflow step that fetches eval-hub-server from a git repository, replacing it with a TestPyPI-based distribution model. The test-e2e target is updated with improved debug logging capabilities, and the E2E fixture now explicitly manages temporary directories for debugging purposes. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as Workflow
participant OldSource as Git Repository
participant NewSource as TestPyPI
participant Test as E2E Test
rect rgba(100, 100, 150, 0.5)
Note over Workflow,Test: Previous Flow (Removed)
Workflow->>Workflow: Matrix includes latest_eval_hub_server
Workflow->>OldSource: Fetch eval-hub-server from Git subdirectory
OldSource-->>Workflow: eval-hub-server binary
Workflow->>Test: Run E2E tests with fetched binary
end
rect rgba(100, 150, 100, 0.5)
Note over Workflow,Test: New Flow (Current)
Workflow->>NewSource: Resolve eval-hub-server from TestPyPI
NewSource-->>Workflow: eval-hub-server package
Workflow->>Test: Run E2E tests with --e2e-debug logging
Test->>Test: Preserve temp directory for debugging
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/conftest.py (1)
169-176:⚠️ Potential issue | 🟠 MajorPotential
UnboundLocalErrorif server startup fails early.If an exception occurs before
server_processis assigned (e.g., duringshutil.copytreeat line 104 or any earlier operation), thefinallyblock will raiseUnboundLocalError: local variable 'server_process' referenced before assignment.🐛 Proposed fix: guard the cleanup with existence check
finally: # Cleanup: terminate the server subprocess - try: - server_process.terminate() - server_process.wait(timeout=5) - except subprocess.TimeoutExpired: - server_process.kill() - server_process.wait() + if "server_process" in dir(): + try: + server_process.terminate() + server_process.wait(timeout=5) + except subprocess.TimeoutExpired: + server_process.kill() + server_process.wait()Alternatively, initialize
server_process = Nonebefore thetryblock:tmpdir = tempfile.mkdtemp(prefix="evalhub-e2e-") + server_process = None try: ... finally: # Cleanup: terminate the server subprocess - try: - server_process.terminate() + if server_process is not None: + try: + server_process.terminate() + server_process.wait(timeout=5) + except subprocess.TimeoutExpired: + server_process.kill() + server_process.wait()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/conftest.py` around lines 169 - 176, The finally block can reference server_process before assignment causing UnboundLocalError; initialize server_process = None prior to the try that starts the server (or alternatively guard the cleanup) and then in the finally only call server_process.terminate()/wait()/kill() if server_process is not None (or if 'server_process' in locals()); update the variables around the server startup code where server_process is created and the cleanup code that calls server_process.terminate(), server_process.wait(), and server_process.kill() accordingly.
🧹 Nitpick comments (1)
tests/e2e/conftest.py (1)
98-100: Temp directory intentionally preserved for debugging — consider documenting cleanup.The switch from
TemporaryDirectory(auto-cleanup) tomkdtemp(manual) preserves the directory after test runs for debugging server logs. This is useful but will accumulate directories over time.Consider adding a comment or log message indicating where the directory is and that it won't be auto-cleaned, or add a conditional cleanup based on test success.
# Create temporary directory for server files (preserved after run for debugging of server logfiles, etc) tmpdir = tempfile.mkdtemp(prefix="evalhub-e2e-") + # NOTE: This directory is intentionally NOT cleaned up to allow post-run debugging. + # Clean up manually or run: find /tmp -name 'evalhub-e2e-*' -type d -mtime +1 -exec rm -rf {} + try: logger.debug(f"\nTemp directory for this run: {tmpdir}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/conftest.py` around lines 98 - 100, The tmpdir is created with tempfile.mkdtemp (variable tmpdir) and left on disk, which can accumulate; update conftest.py to either add a clear inline comment near the tempfile.mkdtemp call explaining the intentional preservation and its path (tmpdir) or emit a log message (e.g., via pytest's logging or print) that shows the tmpdir path and that it is not auto-cleaned, and optionally implement conditional cleanup after the test run (based on a success flag or an environment variable) that removes tmpdir when desired; reference the tempfile.mkdtemp call and the tmpdir variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 10-12: The test selection now uses pytest marker filtering so the
missing `@pytest.mark.e2e` decorators are silently excluding tests; add
`@pytest.mark.e2e` to the TestOCIArtifactPersistenceE2E class in
tests/e2e/test_persist_with_mock_persister.py (so all 3 methods inherit the
marker), and add `@pytest.mark.e2e` to each of the five standalone test functions
in tests/e2e/test_smoke_test_eval_hub_server.py and the four standalone test
functions in tests/e2e/test_eval_hub_server_with_real_config.py so they are
included when running make test-e2e.
---
Outside diff comments:
In `@tests/e2e/conftest.py`:
- Around line 169-176: The finally block can reference server_process before
assignment causing UnboundLocalError; initialize server_process = None prior to
the try that starts the server (or alternatively guard the cleanup) and then in
the finally only call server_process.terminate()/wait()/kill() if server_process
is not None (or if 'server_process' in locals()); update the variables around
the server startup code where server_process is created and the cleanup code
that calls server_process.terminate(), server_process.wait(), and
server_process.kill() accordingly.
---
Nitpick comments:
In `@tests/e2e/conftest.py`:
- Around line 98-100: The tmpdir is created with tempfile.mkdtemp (variable
tmpdir) and left on disk, which can accumulate; update conftest.py to either add
a clear inline comment near the tempfile.mkdtemp call explaining the intentional
preservation and its path (tmpdir) or emit a log message (e.g., via pytest's
logging or print) that shows the tmpdir path and that it is not auto-cleaned,
and optionally implement conditional cleanup after the test run (based on a
success flag or an environment variable) that removes tmpdir when desired;
reference the tempfile.mkdtemp call and the tmpdir variable when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 768188fa-304a-4a8a-bb6e-752be4b6bc46
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/test.ymlMakefilepyproject.tomltests/conftest.pytests/e2e/conftest.py
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: tarilabs <matteo.mortari@gmail.com>
tarilabs
requested review from
gnaulak-redhat,
julpayne,
mariusdanciu,
nbs-rh,
ppadashe-psp,
ruivieira and
scheruku-rh
2 participants
What and why
Refactor and simplify test-e2e using the Dev release of eval-hub-server from TestPypi
Type
Testing
Breaking changes
none
Summary by CodeRabbit
Release Notes
Tests
Chores