build: prepare for test matrix #176
Conversation
WalkthroughThe pull request introduces a new target Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alex-liang3
force-pushed
the
users/alexliang/test-matrix
branch
from
e63b919 to
fb418ec
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
=======================================
Coverage 76.31% 76.31%
=======================================
Files 31 31
Lines 2613 2613
=======================================
Hits 1994 1994
Misses 619 619 ☔ View full report in Codecov by Sentry. |
alex-liang3
force-pushed
the
users/alexliang/test-matrix
branch
from
fb418ec to
667d7e9
Compare
alex-liang3
force-pushed
the
users/alexliang/test-matrix
branch
from
7470358 to
66cc3d4
Compare
![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
🧹 Nitpick comments (1)
Makefile (1)
525-541: Consider integration with test-unit target.The new
ci-test-pythontarget is not included in the main test targets (test,test-unit). Consider whether it should be integrated or kept separate for CI-only use.Options to consider:
- Keep separate (current implementation) - Good for CI-specific flows
- Include in test-unit - Better integration but might slow down local development
- Add a new composite target for CI - Best of both worlds
If you want to integrate it, here's a suggested approach:
test-unit: ## Run unit tests @ echo "Running unit tests..." @ $(MAKE) test-unit-cpp - @ $(MAKE) test-unit-python + @ if [ "${CI}" = "true" ]; then \ + $(MAKE) ci-test-python; \ + else \ + $(MAKE) test-unit-python; \ + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Run Python Unit Tests
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Check C++ Format
🔇 Additional comments (2)
Makefile (2)
525-526: LGTM! Clear and descriptive target documentation.The comment clearly states the prerequisites (dev image and bindings) needed for this target.
529-538: Verify error handling for missing prerequisites.The target assumes the development image and Python packages exist but doesn't verify these prerequisites. Consider adding checks to provide helpful error messages if they're missing.
Run this script to check how other targets handle missing prerequisites:
Consider adding error checks:
ci-test-python: ## Run Python unit tests. Assumes the dev image has already been built, AND that bindings have been built and are available at `packages/python` + @ if [ ! -d "packages/python" ]; then \ + echo "Error: Python packages not found in packages/python. Please build them first."; \ + exit 1; \ + fi @ echo "Running Python unit tests..."
alex-liang3
force-pushed
the
users/alexliang/test-matrix
branch
from
66cc3d4 to
e35b36c
Compare
Adds a new Makefile directive to be used in CI to facilitate unit testing multiple Python versions. This should be merged BEFORE open-space-collective/open-space-toolkit#144, and will only kick in after the latter is merged.
Summary by CodeRabbit