ci: ensure PR testing runs correctly based on conditions #1486
Conversation
ssala034
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
WalkthroughThe pull request modifies the GitHub Actions workflow file to change the output assignment in the Changes
Sequence Diagram(s)sequenceDiagram
participant Filter as Filter Step
participant Changes as Changes Job
participant Test as Test Job
Filter->>Changes: Provides modified_files output
Changes->>Test: Passes should_test (array or empty array)
Test->>Test: Checks if pull request is not a draft
Test->>Test: Determines if tests should run based on should_test
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 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 (
|
![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
🔭 Outside diff range comments (2)
.github/workflows/pr-testing-with-test-project.yml (2)
32-32: 🛠️ Refactor suggestion
⚠️ Potential issueAdjust test job condition for proper evaluation
With the updated output type of
should_test, the current condition:if: ${{ needs.changes.outputs.should_test == 'true' && github.event.pull_request.draft == false }}no longer correctly evaluates whether there are modified files. Consider modifying the condition to check that the modified files array is non-empty. For example:
- if: ${{ needs.changes.outputs.should_test == 'true' && github.event.pull_request.draft == false }} + if: ${{ needs.changes.outputs.should_test != '[]' && github.event.pull_request.draft == false }}This change ensures that tests run only when there are modified files.
15-16: 💡 Verification agent❓ Verification inconclusive
Add missing step identifier for the paths-filter action
The workflow later references
steps.filter.outputs.modified_files(line 12), but the corresponding step (using therohansingh/paths-filteraction) does not have anidassigned. To ensure the outputs can be properly referenced, add an identifier (for example,id: filter) to that step.- - uses: rohansingh/paths-filter@084ca29929a4e7e708a4aa8b790347facfd830c9 # fork with predicate-quantifier fix + - id: filter + uses: rohansingh/paths-filter@084ca29929a4e7e708a4aa8b790347facfd830c9 # fork with predicate-quantifier fix
Action Required: Add Missing Step Identifier in Workflow
To correctly reference the output from the paths-filter action via
steps.filter.outputs.modified_files, please add an identifier to that step. For example, update the changes as shown below:- - uses: rohansingh/paths-filter@084ca29929a4e7e708a4aa8b790347facfd830c9 # fork with predicate-quantifier fix + - id: filter + uses: rohansingh/paths-filter@084ca29929a4e7e708a4aa8b790347facfd830c9 # fork with predicate-quantifier fixThis change ensures that the workflow can correctly reference the outputs of the paths-filter action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-testing-with-test-project.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pr-testing-with-test-project.yml
12-12: property "filter" is not defined in object type {}
(expression)
so the issue of "hanging" tests is still there, because we have part |
|
I'll look into updating the logic. Thanks for the feedback. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pr-testing-with-test-project.yml (1)
41-58: Verify comprehensive conditions in "Determine if tests should run" step.
The step now uses a compound condition that:
- Checks that
needs.changes.outputs.should_test == 'true'(ensuring the paths-filter detected relevant changes),- Reaffirms that the PR is not a draft, and
- Excludes test runs initiated by specific bot-generated pull requests.
This structure is clear and aligns with the objective to trigger tests only when there are meaningful file changes and the PR qualifies for testing. One minor point to consider: the non-draft check appears both at the job level (line 32) and here; while redundant, this may be intentional for increased robustness. If desired, you could refactor to remove the redundancy, but it is acceptable as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-testing-with-test-project.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/pr-testing-with-test-project.yml (1)
Learnt from: derberg
PR: asyncapi/generator#1486
File: .github/workflows/pr-testing-with-test-project.yml:12-12
Timestamp: 2025-04-10T18:27:09.717Z
Learning: The paths-filter GitHub Action (like dorny/paths-filter or its fork rohansingh/paths-filter) outputs the string 'true' when files match a filter pattern, not an array. When using these outputs in workflow conditions, they should be checked with `== 'true'`, not compared against '[]'.
🪛 actionlint (1.7.4)
.github/workflows/pr-testing-with-test-project.yml
12-12: property "filter" is not defined in object type {}
(expression)
🔇 Additional comments (2)
.github/workflows/pr-testing-with-test-project.yml (2)
12-12: Validate output reference forshould_test.
The outputshould_testis now assigned directly using${{ steps.filter.outputs.modified_files }}, which correctly leverages the string output ('true') provided by therohansingh/paths-filteraction. However, ensure that the corresponding paths-filter step explicitly defines anid(e.g.,id: filter) so that referencing it viasteps.filter.outputs.modified_filesis valid and avoids potential static analysis issues.🧰 Tools
🪛 actionlint (1.7.4)
12-12: property "filter" is not defined in object type {}
(expression)
31-32: Simplify job-level condition check for non-draft PRs.
The job-level condition has been simplified toif: ${{ github.event.pull_request.draft == false }}, removing the direct dependency onshould_test. This is acceptable since the detailed file-change check is now deferred to the "Determine if tests should run" step. Please confirm that this separation of concerns (global job execution controlled by draft status and step execution controlled by file changes and additional conditions) is the intended behavior.
|
@derberg The "hanging" tests quickly passed. Please lmk about any changes that should be made. |
Description
Removed wrong equality check on different data types. Now
should_testvariable should be giving the needed value.Related issue(s)
Related to #1345
Summary by CodeRabbit