Skip to content

Feature/improve build pipeline #69

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.

Sign up for GitHub

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

Merged
RHSplinter merged 2 commits into from
Dec 6, 2025
Merged

Feature/improve build pipeline #69

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50a56a2
Update condition for reports
RHSplinter Dec 6, 2025
f41f4d1
Execute e2e tests on main or PR
RHSplinter Dec 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ jobs:
run: npm run build:prod

- name: Run unit tests
id: unit-tests
run: npm run test:prod

- name: Upload Test Results Report
if: always()
- name: Upload Test Result Report

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- name: Upload Test Result Report
- name: Upload Test Results Report

Copilot uses AI. Check for mistakes.
if: steps.unit-tests.conclusion != 'skipped'

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
uses: dorny/test-reporter@fe45e9537387dac839af0d33ba56eed8e24189e8 # v2.3.0
with:
name: Test Results
Expand All @@ -53,7 +54,7 @@ jobs:
report-title: Test Results

- name: Create Code Coverage Summary Report
if: always()
if: steps.unit-tests.conclusion != 'skipped'

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
uses: irongut/CodeCoverageSummary@51cc3a756ddcd398d447c044c02cb6aa83fdae95 #v1.3.0
with:
filename: "test_results/coverage/cobertura-coverage.xml"
Expand All @@ -62,7 +63,7 @@ jobs:
output: "both"

- name: Upload Code Coverage Report
if: always()
if: steps.unit-tests.conclusion != 'skipped'

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
run: |
echo "# Code Coverage" >> $GITHUB_STEP_SUMMARY
if [ -f code-coverage-results.md ]; then
Expand All @@ -83,6 +84,7 @@ jobs:
e2e:
needs: build
runs-on: ubuntu-latest
if: github.ref == 'refs/heads/main' || github.event_name == 'pull_request'

Copilot AI Dec 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.
strategy:
matrix:
browser: [chrome, firefox, edge]
Expand Down