Feature/improve build pipeline#69
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the build pipeline by adding conditional execution logic to optimize workflow runs. It adds an ID to the unit tests step and modifies conditional logic for test reporting steps, while also restricting E2E test execution to specific branches and events.
Key Changes:
- Modified conditional logic for test and coverage reporting steps from
always()to check unit-tests conclusion - Added conditional execution to the E2E job to run only on main branch or pull requests
- Minor naming change to test report upload step
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - name: Upload Test Results Report | ||
| if: always() | ||
| - name: Upload Test Result Report |
There was a problem hiding this comment.
Changed from "Upload Test Results Report" to "Upload Test Result Report" (Results → Result). This makes the naming inconsistent with the test reporter action's name parameter which is "Test Results" (line 51) and the report-title which is also "Test Results" (line 54).
| - name: Upload Test Result Report | |
| - name: Upload Test Results Report |
| - name: Upload Test Results Report | ||
| if: always() | ||
| - name: Upload Test Result Report | ||
| if: steps.unit-tests.conclusion != 'skipped' |
There was a problem hiding this comment.
The condition steps.unit-tests.conclusion != 'skipped' doesn't handle test failures correctly. If the unit tests fail, the conclusion will be 'failure', not 'skipped', so these reporting steps will still run. However, this differs from the original always() behavior which ensures test reports are uploaded even on failure - which is typically desired for debugging.
Issue: With this condition, the steps will run on both success and failure, but not if the step is skipped. However, if a previous step fails (e.g., build or lint), the unit-tests step won't run at all (it will be skipped by default), and these reporting steps won't run either.
Original behavior with always(): Test reports would be uploaded regardless of whether tests passed, failed, or were skipped - providing maximum visibility.
Recommendation: Use if: always() to ensure test reports are always uploaded for debugging purposes, or use if: success() || failure() if you specifically want to exclude the case where unit-tests were skipped. The condition steps.unit-tests.conclusion != 'skipped' is less clear and doesn't protect against the step not running at all (which has no conclusion).
|
|
||
| - name: Create Code Coverage Summary Report | ||
| if: always() | ||
| if: steps.unit-tests.conclusion != 'skipped' |
There was a problem hiding this comment.
The condition steps.unit-tests.conclusion != 'skipped' doesn't handle test failures correctly. If the unit tests fail, the conclusion will be 'failure', not 'skipped', so this step will still run. However, this differs from the original always() behavior which ensures code coverage reports are generated even on failure.
Issue: With this condition, if a previous step fails before unit-tests (e.g., lint or build), the unit-tests step won't run at all, and this coverage reporting step won't run either. The condition also doesn't account for the step not having a conclusion yet.
Recommendation: Use if: always() to ensure coverage reports are always generated for debugging purposes, or use if: success() || failure() if you specifically want to exclude the case where unit-tests were skipped.
|
|
||
| - name: Upload Code Coverage Report | ||
| if: always() | ||
| if: steps.unit-tests.conclusion != 'skipped' |
There was a problem hiding this comment.
The condition steps.unit-tests.conclusion != 'skipped' doesn't handle test failures correctly. If the unit tests fail, the conclusion will be 'failure', not 'skipped', so this step will still run. However, this differs from the original always() behavior which ensures coverage reports are uploaded even on failure.
Issue: With this condition, if a previous step fails before unit-tests (e.g., lint or build), the unit-tests step won't run at all, and this coverage upload step won't run either. The condition also doesn't account for the step not having a conclusion yet.
Recommendation: Use if: always() to ensure coverage reports are always uploaded for debugging purposes, or use if: success() || failure() if you specifically want to exclude the case where unit-tests were skipped.
| e2e: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| if: github.ref == 'refs/heads/main' || github.event_name == 'pull_request' |
There was a problem hiding this comment.
The condition github.event_name == 'pull_request' should work correctly with the workflow triggers. However, there's a subtle issue: the workflow is triggered by pull_request events for the main branch (line 8-10), and by push events (with path filters).
Potential Issue: When code is pushed directly to main, the condition github.ref == 'refs/heads/main' will be true, so E2E tests will run. However, when code is pushed to other branches (that don't match the path-ignore filters), the E2E job won't run because neither condition will be true.
Recommendation: This might be intentional (running E2E tests only for main branch pushes and PRs to main), but if you want to run E2E tests for all pushes, consider using github.event_name == 'push' || github.event_name == 'pull_request' instead. If the current behavior is intended, consider adding a comment to clarify the logic.
No description provided.