chore: speed up pr-check-test workflow runtime - #1895
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplit CI into three jobs: a 2-shard integration-tests job (fixed Python 3.12), a unit-tests job (Python 3.10–3.14 matrix), and a test-summary job. Added proto generation, Hiero Solo init, shard-aware test discovery/execution, exit-code propagation, and improved failure logging/aggregation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/PR
participant GH as GitHub Actions Runner
participant Proto as Proto Generation
participant Solo as Hiero Solo Prep
participant TestsI as Integration Tests (Shard)
participant TestsU as Unit Tests (Matrix)
participant Summary as Test-Summary Job
Dev->>GH: push PR / trigger workflow
GH->>Proto: generate proto files
GH->>Solo: initialize Hiero Solo
GH->>TestsI: start integration-tests (set SHARD_INDEX/SHARD_TOTAL)
TestsI->>GH: discover tests, partition by shard
TestsI->>GH: run shard subset, record exit_code & ERROR logs
GH->>TestsU: run unit-tests matrix (3.10–3.14), generate protos per job
TestsU->>GH: run tests, record exit_code & ERROR logs
GH->>Summary: test-summary (needs: integration-tests, unit-tests) collects outputs
Summary->>Dev: post aggregated test summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
5cf77c2 to
08f40bf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/pr-check-test.yml (1)
16-18:⚠️ Potential issue | 🟠 MajorScope
GITHUB_TOKENpermissions to least privilege.
actions: writeis broader than needed for this workflow and increases risk on trusted-branch runs. No step in this workflow requires writing Actions resources.🔧 Suggested fix
permissions: contents: read - actions: writeAs per coding guidelines, “
permissions:must be explicitly declared and scoped to the minimum needed.”
|
Applied, thanks. |
There was a problem hiding this comment.
Hi @adityagiri3600, nice work overall. I do have some concerns about certain points
I think the summary is misleading here...the job is being treated as a success when it should actually fail
+the project isn't installed for tests.. what do you think?
|
[commit-verification-bot]
View your commit verification status: Commits Tab. To achieve verified status, please read: Remember, you require a GPG key and each commit must be signed with: Thank you for contributing! From the Hiero Python SDK Team |
e83a8fc to
309231d
Compare
|
good catch @MonaaEid , thanks @manishdait for finding the fix, fixing it now |
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. Quick Fix for CHANGELOG.md ConflictsIf your conflict is only in CHANGELOG.md, you can resolve it easily using the GitHub web editor:
For all other merge conflicts, please read: Thank you for contributing! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr-check-test.yml (1)
16-18:⚠️ Potential issue | 🟠 MajorScope
GITHUB_TOKENto least privilege;actions: writeappears unnecessary.This workflow does not show a write operation against Actions APIs, so retaining
actions: writebroadens risk on trusted-branch runs without clear need.🔧 Proposed fix
permissions: contents: read - actions: writeAs per coding guidelines, “
permissions:must be explicitly declared and scoped to the minimum needed. Flag over-permissioned or under-permissioned workflows, and any reliance on broad default permissions.”
exploreriii
added
reviewer: maintainer
do you want me to do this here? or later in a separate PR? |
|
separate probably |
Signed-off-by: Aditya Giri <74224708+adityagiri3600@users.noreply.github.com>
exploreriii
added
status: needs triage review
reviewer: maintainer
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1895 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 141 141
Lines 9146 9146
=======================================
Hits 8555 8555
Misses 591 591 🚀 New features to boost your workflow:
|
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
This looks good to me, can you run some tests please and send us the results
exploreriii
added
status: needs developer testing
and removed
status: needs triage review
reviewer: maintainer
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
exploreriii
added
reviewer: maintainer
|
Request review @manishdait please |
6 participants
Description:
Optimize PR CI runtime in
.github/workflows/pr-check-test.ymltest-summaryjob to aggregate outcomes and fail workflow if any of the test job failspaths-ignorefor docs/markdown-only changes to avoid unnecessary runs**Related issue(s)*:
Fixes #1878
Notes for reviewer:
Checklist