🌱 prove we can use labels to enable extra testing#984
🌱 prove we can use labels to enable extra testing#984djzager wants to merge 1 commit intokonveyor:mainfrom
Conversation
WalkthroughAdds two composite GitHub Actions and refactors CI to use them. .github/actions/setup-e2e/action.yml prepares an E2E environment (Node.js, Java, VSCode, X11/D-Bus), locates VSIX artifacts, and exposes Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-07-16T14:40:48.171ZApplied to files:
📚 Learning: 2025-07-21T15:15:59.771ZApplied 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). (4)
🔇 Additional comments (14)
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
🧹 Nitpick comments (1)
.github/actions/collect-e2e-artifacts/action.yml (1)
1-48: Clean, focused artifact collection action with good error resilience. The implementation correctly usesalways()guards to ensure artifacts are collected and uploaded regardless of test outcomes. Conditional directory checks are safe and the extension logs fallback provides diagnostic clarity.For future flexibility, consider parameterizing the retention period as an optional input (defaulting to 7 days).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/collect-e2e-artifacts/action.yml(1 hunks).github/actions/setup-e2e/action.yml(1 hunks).github/workflows/ci-repo.yml(1 hunks)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (6)
.github/workflows/ci-repo.yml (4)
218-230: Setup action properly integrated with output consumption. Thesetup-e2eaction is correctly invoked with an id, and downstream steps consume its outputs for VSIX paths. The environment variables are properly passed to E2E test steps.
241-245: Good refactoring to use dedicated artifact action. Replacing inline artifact collection with the composite action improves consistency and maintainability. Thealways()guard is preserved.
287-289: Publish job dependency correctly scoped to required test only. Thepublishjob depends only ontest-e2e, nottest-custom-binary. This allows publishing to proceed even when the optional custom-binary test is skipped (correct for optional testing model).Confirm this is the intended behavior: should custom-binary test failures block publishing? Currently, a passing test-e2e can publish even if test-custom-binary fails (when label is present). If custom-binary test should gate publishing, update line 287 to
needs: [test-e2e, test-custom-binary]and adjust the job condition accordingly.
247-282: Label-based conditional test job correctly implemented and verified. Thetest-custom-binaryjob properly triggers on PR labeltest/custom-binary, the test file exists at the referenced path, and the GitHub Actions conditional syntax is valid. The job mirrors the standard E2E test structure while maintaining its own artifact namespace..github/actions/setup-e2e/action.yml (2)
36-109: E2E environment setup is comprehensive and well-structured. Node.js, Java, VSCode, X11, and D-Bus configuration follows best practices for headless testing infrastructure.
116-155: I need to verify the vsce naming behavior and check for any custom configuration:Based on my verification of the codebase and vsce behavior:
VSIX discovery is sound—no changes needed.
The default vsce output format is
{name}-{version}.vsix(e.g.,konveyor-0.4.0.vsix), which matches the glob patterns used in the action. The extension names from package.json (konveyor,konveyor-java,konveyor-javascript) combined withhead -n 1reliably selects the correct VSIX file, and the error handling withif [ ! -f "$CORE_VSIX" ]properly validates file existence and provides helpful debugging output. The approach is well-suited for CI/CD workflows.
djzager
force-pushed
the
mo-tests-mo-problems
branch
from
d44d6e9 to
6c4c2fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/actions/setup-e2e/action.yml (1)
63-77: Questionable packages in X11 dependencies (duplicate of prior review).Lines 76 includes
nickle cairo-5c, which appear unrelated to VSCode E2E testing. See prior review feedback to remove unless there's a specific upstream dependency chain requiring them.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/collect-e2e-artifacts/action.yml(1 hunks).github/actions/setup-e2e/action.yml(1 hunks).github/workflows/ci-repo.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/collect-e2e-artifacts/action.yml
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (6)
.github/workflows/ci-repo.yml (2)
218-245: Refactor to composite actions improves modularity and maintainability. The centralization of E2E setup logic and artifact collection into reusable actions is a solid improvement for code organization.
247-282: Label-driven conditional E2E test job is well-structured. The test-custom-binary job properly uses the conditional label check and integrates correctly with the setup-e2e action outputs for VSIX paths..github/actions/setup-e2e/action.yml (4)
1-31: Action metadata is well-structured with appropriate input/output definitions.
33-61: VSCode and Java setup steps are correct and follow best practices.
97-115: Verification and artifact setup logic is sound.
116-155: VSIX discovery and validation logic is sound, with appropriate error handling.The glob pattern approach works well for this context. The explicit validation and detailed error messages (including directory listing on failure) make debugging straightforward.
djzager
force-pushed
the
mo-tests-mo-problems
branch
from
6c4c2fa to
281e3b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/collect-e2e-artifacts/action.yml(1 hunks).github/actions/setup-e2e/action.yml(1 hunks).github/workflows/ci-repo.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/collect-e2e-artifacts/action.yml
- .github/actions/setup-e2e/action.yml
⏰ 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). (1)
- GitHub Check: Lint
🔇 Additional comments (2)
.github/workflows/ci-repo.yml (2)
218-246: Approve test-e2e job refactoring to use composite actions.The refactoring to use the
setup-e2ecomposite action and the new artifact collection action is clean. Output references (steps.setup.outputs.core-vsix,steps.setup.outputs.java-vsix) are consistent across steps, and the artifact collection maintains theif: always()pattern for reliability.
247-283: Approve test-custom-binary job structure and conditional logic.The new job is well-structured:
- The condition on line 253 correctly gates execution to labeled PRs:
if: github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'test/custom-binary')- Reuses the established setup and artifact collection actions for consistency
- The additional asset collection step (lines 262-268) for custom binary test dependencies is appropriately isolated
Separate files makes it cleaner Signed-off-by: David Zager <[email protected]>
djzager
force-pushed
the
mo-tests-mo-problems
branch
from
281e3b9 to
0c72c4e
Compare
|
Never worked quite how I had in mind. |
1 participant
Demonstrate that we can make certain tests run when the PR is labelled a certain way.
Summary by CodeRabbit