ci: isolate nightly package tests from source tree#3274
Conversation
dierksen
requested review from
aleozlx,
bkryu,
cyx-6,
jimmyzho,
kahyunnam,
nv-yunzheq,
saltyminty,
samuellees,
sricketts,
yongwww,
yyihuang and
yzh119
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe nightly build test script computes SOURCE_WORKSPACE, creates a temporary TEST_RUN_DIR and EXIT trap, copies only ChangesNightly Test Isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/task_test_nightly_build.sh (1)
60-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelative
FLASHINFER_JIT_CACHE_REPORT_FILEpaths are silently deleted by the EXIT trap.The subshell at line 67
cds intoTEST_RUN_DIR, so any relative path inFLASHINFER_JIT_CACHE_REPORT_FILEresolves there. The EXIT trap then runsrm -rf "${TEST_RUN_DIR}", destroying the file before the caller can read it. No error is raised — the report is silently lost.🛡️ Proposed fix — resolve to absolute path before the cd
if [ -n "${FLASHINFER_JIT_CACHE_REPORT_FILE}" ]; then + FLASHINFER_JIT_CACHE_REPORT_FILE="$(realpath -m "${FLASHINFER_JIT_CACHE_REPORT_FILE}")" export FLASHINFER_JIT_CACHE_REPORT_FILE fi
realpath -mresolves relative-to-CWD without requiring the file to already exist, so it is safe to call before the report is written.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/task_test_nightly_build.sh` around lines 60 - 67, The FLASHINFER_JIT_CACHE_REPORT_FILE may be a relative path and gets deleted by the EXIT trap that removes TEST_RUN_DIR because the subshell (cd "${TEST_RUN_DIR}" && bash ...) changes CWD; to fix, if FLASHINFER_JIT_CACHE_REPORT_FILE is set, resolve it to an absolute path before entering the subshell (use a safe resolver such as realpath -m or similar) and then export the resolved absolute path so the file is created outside TEST_RUN_DIR and survives the EXIT trap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/task_test_nightly_build.sh`:
- Around line 60-67: The FLASHINFER_JIT_CACHE_REPORT_FILE may be a relative path
and gets deleted by the EXIT trap that removes TEST_RUN_DIR because the subshell
(cd "${TEST_RUN_DIR}" && bash ...) changes CWD; to fix, if
FLASHINFER_JIT_CACHE_REPORT_FILE is set, resolve it to an absolute path before
entering the subshell (use a safe resolver such as realpath -m or similar) and
then export the resolved absolute path so the file is created outside
TEST_RUN_DIR and survives the EXIT trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fcd95b8-4898-46f7-8f72-8bd664b7ba32
📒 Files selected for processing (1)
scripts/task_test_nightly_build.sh
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces test isolation for nightly builds by creating a temporary directory, copying necessary test files into it, and executing tests from that location. This ensures that the tests run against the installed distribution rather than being shadowed by the local source directory. The review feedback suggests a more robust way to define the workspace root by deriving it from the script's location rather than the current working directory.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
📌 Description
🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Summary by CodeRabbit