-
Notifications
You must be signed in to change notification settings - Fork 84
feat(ci): Add task for package integration tests. #1664
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
feat(ci): Add task for package integration tests. #1664
Conversation
…ig file; address comments.
…er-level clp-config.yml
…E_CONFIGS other than clp-text and clp-json
WalkthroughAdded a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-08-16T10:24:29.316ZApplied to files:
📚 Learning: 2025-09-25T19:26:32.436ZApplied to files:
📚 Learning: 2025-08-17T16:10:38.722ZApplied to files:
📚 Learning: 2025-10-13T03:32:19.293ZApplied to files:
📚 Learning: 2025-07-07T17:41:15.655ZApplied to files:
📚 Learning: 2025-10-20T21:05:30.417ZApplied to files:
📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (3)
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 |
junhaoliao
left a 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.
for the tile, how about:
feat(ci): Add task for package integration test.
taskfiles/tests/integration.yaml
Outdated
| deps: | ||
| - task: "::package" |
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.
just curious, since we're not passing any parameters to ::package, why don't we just write it as
| deps: | |
| - task: "::package" | |
| deps: ["::package"] |
(i don't know much about the background but i also see the other deps in this taskfile use a similar syntax
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.
I was following the syntax from the other deps; @Bill-hbrhbr what do you think?
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.
I don't have a strong opinion, but what Junhao's suggesting is cleaner.
Let's just be consistent within this file. So if you update this spot, make sure to update the core task as well.
task tests:integration:package to run all package integration tests.Co-authored-by: Junhao Liao <junhao@junhao.ca>
Bill-hbrhbr
left a 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.
Actually thought this PR could fit right into the dev docs PR. But I'm not against breaking it up. So commenting for future considerations.
Description
This PR introduces the command
task tests:integration:package, which runsuv run pytest -m packageunder the hood.PR dependencies
This PR depends on the following being merged:
Checklist
breaking change.
Validation performed
Ran the following command:
Both the
clp-jsonandclp-texttests passed.Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.