[CI] Split Chromedriver tests into parallel jobs #18603
Conversation
WalkthroughRenames several CI E2E job names to "Tests (...)", adds JavaScript E2E workflows and matrix entries, consolidates Behat CLI/API/UI flows with rerun/manifest/log steps and BuildTestAppAction database input, and adds a Behat AfterStep delay hook reading BEHAT_STEP_DELAY. Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as Repository
participant MatrixJSON as matrix.json
participant GetMatrix as get-matrix job
participant BehatJS as behat-js job
participant BehatPanther as behat-panther job
participant Artifacts as Artifacts
Note over Repo,MatrixJSON: CI JS workflow triggered (type: minimal|full)
GetMatrix->>MatrixJSON: read `js` / `js-panther` entries
MatrixJSON-->>GetMatrix: emit matrix groups
GetMatrix->>BehatJS: supply matrix (groups)
GetMatrix->>BehatPanther: supply panther matrix
par JS groups
BehatJS->>BehatJS: checkout → restore cache → build app → run Behat (group tags)
BehatJS-->>Artifacts: upload logs on failure
and Panther groups
BehatPanther->>BehatPanther: setup Chrome → build app → run Panther Behat
BehatPanther-->>Artifacts: upload logs on failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/chromedriver-groups.json (1)
20-21: Document the convention for adding new test groups.The "Remaining" group uses extensive negations (13 tags) to capture tests not explicitly categorized. As the codebase grows and new feature tags are added, this list must be manually updated. Consider documenting the convention that new feature tests should be assigned to a specific group (rather than relying on the "Remaining" fallback) to reduce maintenance burden.
.github/workflows/ci_e2e-js.yaml (1)
20-27: Minor: Unused matrix output.The get-matrix job exports three outputs (matrix, chromedriver-matrix, panther-matrix), but line 25's
matrixoutput appears unused. Clarify the intent: if it's intentionally reserved for future use, consider adding a comment; if unused, it can be removed to simplify the workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(0 hunks).github/workflows/ci_e2e-unstable.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci_e2e-mysql.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-unstable.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 86-87: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: auto-merge
🔇 Additional comments (5)
.github/workflows/ci_e2e-unstable.yaml (1)
84-93: Verify the intentional narrowing of non-JS test coverage in the unstable workflow.The non-JS Behat step was reduced from running a broader set of tests (across tagged suites) to testing only a single scenario (
seeing_not_found_page_in_the_admin_panel.feature:14). The debug step is a useful addition, but ensure this narrowed scope is intentional and doesn't unintentionally reduce coverage of unstable scenarios..github/workflows/ci__minimal.yaml (1)
40-45: LGTM!The new e2e-js job is properly integrated with correct dependencies, naming, and workflow reference. The placement aligns with the workflow structure.
.github/workflows/ci_e2e-js.yaml (3)
56-74: LGTM! The matrix generation strategy is sound.The jq-based matrix construction cleverly parallelizes Chromedriver tests across five groups while running both Chromedriver and Panther on a single PHP version (8.3), as documented. The design rationale—that JS tests verify browser interactions, not PHP compatibility—is well-motivated and aligns with the PR goal of reducing timeout issues.
91-102: LGTM! Per-branch Node version handling is well-implemented.The script correctly maps branch versions to Node versions (1.14→20.x, 2.0→22.x, default→24.x), ensuring backward compatibility across branches while using current Node for the main branch.
75-158: I'm unable to access the repository due to a clone failure. However, I can analyze the review comment based on the context provided and general knowledge of Behat tag syntax.Let me provide my assessment:
Verify the tag construction logic and Behat parsing capability for complex "Remaining" group negations
Based on the code snippet provided at line 144:
TAGS="@mink:chromedriver&&(${{ matrix.group.tags }})&&~@todo&&~@cli&&~@failing"The tag syntax appears structurally sound for Behat. The parentheses correctly group the matrix-interpolated tags, and the negation operators (
~@) are properly formatted. However, without access to the actualchromedriver-groups.jsonfile and the specific "Remaining" group tag list (claimed to have 13 tags), I cannot definitively verify:
- Whether the interpolated tag expression produces valid Behat syntax once the matrix variables are resolved
- The actual count and complexity of negations in the "Remaining" group
- Whether Behat's parser can successfully handle the full complex expression with all negations
Regarding line 86-87 (database credentials): The comment correctly notes this is a false positive—these are standard dummy credentials used in CI environments and are not a security concern.
a00bf49 to
452d1ed
Compare
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci_e2e-js.yaml (2)
104-112: Consider refactoring duplicate checkout logic.The same conditional checkout pattern (with/without branch input) is repeated three times across this file (lines 29–37, 104–112, 190–198). This creates maintenance burden and violates DRY.
Consider extracting this into a reusable composite action or composite step to reduce duplication:
# Option 1: Create .github/actions/conditional-checkout/action.yaml name: Conditional Checkout description: Checkout with optional branch override inputs: branch: description: Branch to checkout required: false default: "" runs: using: composite steps: - name: Checkout with branch if: inputs.branch != '' uses: actions/checkout@v4 with: ref: ${{ inputs.branch }} - name: Checkout if: inputs.branch == '' uses: actions/checkout@v4Then replace all three instances with:
- uses: ./.github/actions/conditional-checkout with: branch: ${{ inputs.branch }}Also applies to: 190-198
160-244: Panther job mirrors chromedriver structure appropriately.The panther job follows a similar pattern to chromedriver with appropriate adjustments (chrome_version: stable, different Behat tags). The comment correctly notes this is temporary pending panther migration to chromedriver. However, this introduces additional code duplication (Node.js branching logic, checkout logic, cache setup, app build parameters).
If the panther job will persist beyond the current transition, consider extracting shared setup (Node.js versioning, checkout, cache, build) into a composite action or shared workflow to reduce duplication across behat-chromedriver and behat-panther jobs.
For now, since panther is transitional, deferring this refactor is reasonable, but document when/how the migration to chromedriver is planned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci_e2e-mysql.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/chromedriver-groups.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 86-87: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (7)
.github/workflows/ci__minimal.yaml (1)
40-45: Integration looks good.The new e2e-js job follows the established pattern consistently: proper dependency on static-checks, reuses the new ci_e2e-js workflow, and passes the type parameter. Placement and naming are consistent with other E2E jobs.
.github/workflows/ci_e2e-js.yaml (6)
1-19: Workflow metadata and inputs configured correctly.Triggers, input definitions, and permissions follow best practices. The optional branch input and required type input enable flexible calling from ci__minimal.yaml and other workflows.
84-87: Database credentials flagged by security scanner (expected CI pattern, but document the rationale).Checkov flags the hardcoded DATABASE_URL credentials (CKV_SECRET_4). While these are standard test database credentials (root:root on localhost) and appropriate for CI ephemeral environments, this pattern warrants a brief comment or documentation explaining:
- These are test-only ephemeral credentials, not production secrets
- CI runner is isolated and credentials cannot be exfiltrated
- This aligns with standard GitHub Actions testing patterns
Consider adding an inline comment to preempt future scanner warnings.
Verify whether these credentials should be sourced from environment secrets instead (though likely unnecessary for CI isolation). If the current approach is intentional, add a code comment explaining the rationale.
91-102: Node.js version branching logic is correct.Per-branch Node.js version selection (20.x for 1.14, 22.x for 2.0, 24.x default) appropriately targets platform compatibility. Logic is clear and maintainable.
142-147: Behat tag strategy and retry logic are sound.Tag composition correctly filters by driver, group, and exclusions. The three-attempt retry strategy (initial + 2 reruns) balances flakiness tolerance with CI time without resorting to unbounded retries.
21-73: Verify that chromedriver-groups.json schema matches jq expectations.The get-matrix job assumes chromedriver-groups.json has a
.groupsarray where each group object containsnameandtagsproperties (used in lines 78, 142, 153). Confirm this schema is defined correctly in chromedriver-groups.json before merge.
21-73: The repository clone failed, preventing me from accessing the configuration files and validating the jq transformations. Since I cannot verify the facts this review comment depends on through automation, I need to assess the review comment based on its structural and logical soundness.Unable to verify configuration files and jq transformations due to repository access failure.
The review comment requests verification of matrix configuration files (
chromedriver-groups.jsonandmatrix.json) and jq transformations, but the repository clone operation failed. Without filesystem access, I cannot confirm:
- Whether both configuration files exist in
.github/workflows/- JSON validity and structure of these files
- Whether the jq transformations produce the expected matrix shapes
- Whether the workflow YAML correctly references these files
The logic described in the review comment is sound (using first PHP version × all groups for chromedriver, single entry for panther), but these conclusions depend on file existence and structure that I cannot verify.
2b8437a to
32277be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/chromedriver-groups.json (1)
1-24: Fragile maintenance pattern for "Remaining" group tests.The "Remaining" group uses a negation chain that must be kept in sync with all tags in the first four groups. Any new test tags will automatically fall here (as documented in PR objectives), but developers must manually update the negation expression if coverage gaps need to be reclassified. This creates a silent maintenance burden.
Consider documenting this pattern clearly or implementing a build-time validation to ensure the negation is exhaustive and consistent.
.github/workflows/ci_e2e-js.yaml (1)
90-101: Extract Node.js version selection to reduce duplication.Node.js version selection logic is duplicated verbatim in both
behat-chromedriver(lines 90–101) andbehat-panther(lines 176–187) jobs. This violates DRY and creates a maintenance burden if version mappings change.Extract this logic to a reusable composite action or define it once at the workflow level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci_e2e-mysql.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 85-86: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
- GitHub Check: Static checks / PHP 8.4, Symfony ~7.3.0
🔇 Additional comments (4)
.github/workflows/ci_e2e-js.yaml (4)
39-51: Verify matrix.json structure and integration.The
get-matrixstep reads matrix from.github/workflows/matrix.jsonat${{ inputs.type }}.e2e-mysql, and the chromedriver matrix generation (line 62) takes only the first entry:$matrix.include[0]. This assumes matrix.json always has at least one element ininclude.Confirm that matrix.json is always present and structured as expected, and consider adding a check or documentation about the single-entry assumption.
53-72: Verify Behat tag syntax in matrix generation.The chromedriver matrix combines a database config with group objects:
$m + { group: $g }(line 62). This creates matrix entries with a nestedgroupobject containingnameandtags. The tags are later used in line 143 as${{ matrix.group.tags }}.Verify that this nested structure works correctly with GitHub Actions matrix expansion and that the tag expressions remain valid when embedded in the Behat command.
141-146: Verify Behat tag syntax in chromedriver test execution.Line 143 constructs tags as:
@mink:chromedriver&&(${{ matrix.group.tags }})&&~@todo&&~@cli&&~@failingThis embeds
matrix.group.tagsdirectly, which contains expressions like@managing_products(line 5 of chromedriver-groups.json) or longer OR expressions like@managing_taxons||@managing_shipping_methods||...(line 9).Verify that:
- The substitution of complex tag expressions (with
||inside the parentheses) is valid Behat syntax.- Tag precedence and grouping are correctly evaluated (especially where "Remaining" group uses
&&and~).Test with a sample group to ensure the final tag string is parsed as intended.
159-226: LGTM on behat-panther job structure.The panther job mirrors chromedriver well, with appropriate differences (no group matrix, simpler tags at line 172). However, the same code duplication concerns from the chromedriver section apply here (Node.js version logic, cache key construction).
Once Node.js version selection is extracted and DB credentials hardcoding is addressed, this job can be approved.
0409530 to
169d8d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/ci_e2e-js.yaml (2)
83-87: Migrate hardcoded database credentials to GitHub Secrets.The
DATABASE_URLenvironment variable contains hardcoded test credentials (root:root). While these are disposable CI credentials, this pattern is flagged by security scanning (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets to remove credentials from version control history and CI logs.Consider moving credentials to repository/organization secrets and constructing the URL dynamically:
env: DATABASE_URL: "mysql://${{ secrets.DB_TEST_USER }}:${{ secrets.DB_TEST_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Then create
DB_TEST_USERandDB_TEST_PASSWORDsecrets in the repository settings with valuesrootandrootrespectively.Alternatively, if the credentials are considered non-sensitive in a test-only CI environment, document this decision explicitly in a comment and add a Checkov skip directive if your organization permits it.
175-180: Same credentials issue applies to behat-panther job.The
DATABASE_URLon line 177 also contains hardcoded test credentials. Apply the same GitHub Secrets migration recommended for the behat-chromedriver job to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci_e2e-mysql.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/chromedriver-groups.json
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 85-86: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (2)
- GitHub Check: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
🔇 Additional comments (2)
.github/workflows/ci_e2e-js.yaml (1)
1-250: Workflow structure and matrix-driven parallelization is sound.The use of a separate matrix job (
get-matrix) to generate and expose chromedriver groups and PHP versions is a clean approach to handling the complexity of splitting tests into parallel groups. The conditional branch checkout and per-branch Node.js versioning are well-implemented..github/workflows/ci__minimal.yaml (1)
40-45: Job configuration is properly integrated into minimal CI workflow.The new
e2e-jsjob follows the established pattern for E2E workflow calls, correctly depends onstatic-checks, and passes thetype: minimalparameter to the new CI E2E JavaScript workflow. This maintains consistency with existing E2E jobs and proper dependency sequencing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci__full_2_2.yaml (1)
86-86: Add e2e-js job to notify-about-build-status dependencies.The new e2e-js job (lines 56-65) is not listed in the
needsarray for thenotify-about-build-statusjob (line 86). This causes the notification to be sent before the browser tests complete, and the Slack message won't include the e2e-js job result.Add
e2e-jsto the needs list and update the Slack notification to report its status:notify-about-build-status: if: ${{ always() }} name: "Notify about build status" - needs: [static-checks, e2e-mariadb, e2e-mysql, e2e-pgsql, packages] + needs: [static-checks, e2e-mariadb, e2e-mysql, e2e-pgsql, e2e-js, packages] runs-on: ubuntu-latest timeout-minutes: 5 steps: ... - name: "Notify on Slack" ... with: ... text: | *<https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }} | ${{ github.workflow }} #${{ github.run_number }} build on ${{ github.repository }} repository has been completed for ${{ steps.process-data.outputs.branch }} branch.>* ${{ needs.static-checks.result == 'success' && ':+1:' || ':x:' }} Static Checks ${{ needs.e2e-mariadb.result == 'success' && ':+1:' || ':x:' }} End-to-End (MariaDB) ${{ needs.e2e-mysql.result == 'success' && ':+1:' || ':x:' }} End-to-End (MySQL) ${{ needs.e2e-pgsql.result == 'success' && ':+1:' || ':x:' }} End-to-End (PostgreSQL) + ${{ needs.e2e-js.result == 'success' && ':+1:' || ':x:' }} Behat Browser (MySQL) ${{ needs.packages.result == 'success' && ':+1:' || ':x:' }} PackagesAlso applies to: 56-65
.github/workflows/ci__full_2_1.yaml (1)
86-86: Add e2e-js job to notify-about-build-status dependencies.The new e2e-js job (lines 56-65) is not listed in the
needsarray for thenotify-about-build-statusjob (line 86). This causes the notification to be sent before the browser tests complete, and the Slack message won't include the e2e-js job result.Apply the same fix as in ci__full_2_2.yaml: add
e2e-jsto the needs list and update the Slack notification text to report its status.Also applies to: 56-65
♻️ Duplicate comments (2)
.github/workflows/ci_e2e-js.yaml (2)
141-153: Add validation for empty GROUP_TAGS before running Behat.As flagged in the previous review, if
matrix.group.tagsis empty or undefined, the for loop (line 147) will iterate zero times and the job will complete successfully without running any tests. This silent pass could mask configuration errors inchromedriver-groups.json.Add an explicit validation step before the loop:
- name: Run Behat (Chromedriver - ${{ matrix.group.name }}) env: GROUP_TAGS: ${{ matrix.group.tags }} run: | + if [ -z "$GROUP_TAGS" ]; then + echo "ERROR: GROUP_TAGS is empty for group '${{ matrix.group.name }}'. Check chromedriver-groups.json configuration." + exit 1 + fi + # Split group tags by comma and run behat for each IFS=',' read -ra TAGS_ARRAY <<< "$GROUP_TAGS" for TAG in "${TAGS_ARRAY[@]}"; do
46-51: BLOCKED: Missing chromedriver-groups.json configuration file.The workflow references
.github/workflows/chromedriver-groups.jsonat lines 46-51, but this file does not exist in the repository (as flagged in the previous review). The "Get chromedriver groups" step will fail silently, preventing the chromedriver test matrix from being generated.Ensure that
.github/workflows/chromedriver-groups.jsonis added to the PR with the required structure defining the five groups (Products, Taxonomy, Promotions, Checkout, Remaining) and add error handling to the step to fail loudly if the file is missing:- name: "Get chromedriver groups" id: chromedriver-groups uses: notiz-dev/github-action-json-property@release with: path: '.github/workflows/chromedriver-groups.json' prop_path: 'groups' + + - name: "Validate chromedriver groups" + run: | + if [ -z "${{ steps.chromedriver-groups.outputs.prop }}" ]; then + echo "ERROR: Failed to read chromedriver groups from .github/workflows/chromedriver-groups.json" + exit 1 + fiAlternatively, if the file is deliberately missing, define the groups inline in this workflow or validate the path at workflow parse time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci__full_2_1.yaml(2 hunks).github/workflows/ci__full_2_2.yaml(2 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci__full_2_1.yaml.github/workflows/ci__full_2_2.yaml.github/workflows/ci__minimal.yaml.github/workflows/ci_e2e-mysql.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 85-86: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.4, Symfony ~7.3.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/ci_e2e-js.yaml (3)
46-51:⚠️ Duplicate:chromedriver-groups.jsonfile is missing.This issue was previously identified: the workflow references
.github/workflows/chromedriver-groups.jsonwhich does not exist in the repository. When the "Get chromedriver groups" step executes, the action will fail without clear error messaging.Ensure the file is committed to the repository. If this file is not part of the PR, add it with the five groups structure (Products, Taxonomy, Promotions, Checkout, Remaining) as described in the PR objectives.
If for some reason the file should be generated dynamically instead, replace this external file dependency with either:
- An embedded default groups JSON in the workflow itself
- A validated input parameter
89-93:⚠️ Duplicate: Migrate hardcoded database credentials to GitHub Actions secrets.Hardcoded credentials (
root:root) in theDATABASE_URLenvironment variable at line 91 (and line 183 in the Panther job) should be migrated to GitHub Actions secrets. While these are test-only credentials in a disposable CI environment, this pattern is flagged by security tooling.Store credentials in repository secrets and construct the URL dynamically, as previously suggested in earlier review comments. Ensure repository secrets
DB_USERandDB_PASSWORDare configured with test credentials.
147-159:⚠️ Duplicate: Add safety check for empty group tags to prevent silent passes.If
matrix.group.tagsis empty or undefined, the for loop will execute zero iterations and the job completes successfully without running any Behat tests. This silently masks configuration errors inchromedriver-groups.json.Add an explicit guard before the loop:
- - name: Run Behat (Chromedriver - ${{ matrix.group.name }}) - env: - GROUP_TAGS: ${{ matrix.group.tags }} - run: | - # Split group tags by comma and run behat for each - IFS=',' read -ra TAGS_ARRAY <<< "$GROUP_TAGS" - for TAG in "${TAGS_ARRAY[@]}"; do + - name: Run Behat (Chromedriver - ${{ matrix.group.name }}) + env: + GROUP_TAGS: ${{ matrix.group.tags }} + run: | + # Validate that tags are defined + if [ -z "$GROUP_TAGS" ]; then + echo "ERROR: GROUP_TAGS is empty for group '${{ matrix.group.name }}'" + exit 1 + fi + + # Split group tags by comma and run behat for each + IFS=',' read -ra TAGS_ARRAY <<< "$GROUP_TAGS" + for TAG in "${TAGS_ARRAY[@]}"; do
🧹 Nitpick comments (1)
.github/workflows/ci_e2e-js.yaml (1)
96-107: Consider extracting shared Node.js version selection logic.The branch-to-Node.js version mapping is duplicated identically in both job steps (lines 96-107 and 189-200). While not a blocker, this duplication could become maintenance burden if version mappings change.
Consider extracting to a shared step or creating a reusable composite action for this logic if the workflow grows. For now, this is acceptable.
Also applies to: 189-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci_e2e-js.yaml(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 91-92: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
- GitHub Check: Static checks / PHP 8.5, Symfony ~7.4.0
🔇 Additional comments (2)
.github/workflows/ci_e2e-js.yaml (2)
1-20: ✓ Workflow structure is sound.The workflow design for reusable orchestration of Behat browser tests is appropriate. Permission scoping to
contents: readis correct, optional branch override is defensive, andfail-fast: falseensures all matrix combinations execute despite individual failures.
134-146: ✓ Build, test execution, and artifact handling are appropriate.The use of
BuildTestAppAction@v3.0.1withe2e_js: "yes"is correct for browser testing. Timeout values (25min for Chromedriver, 20min for Panther) are reasonable. Artifact naming includes helpful matrix details (group, PHP, Symfony, MySQL versions) for debugging failures. The configuration flags (if-no-files-found: ignore,overwrite: true) are sensible for CI contexts.Also applies to: 227-239, 161-170, 247-256
5b2d781 to
6cb3689
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/ci_e2e-js.yaml (3)
89-91: Migrate hardcoded database credentials to GitHub Actions secrets.Line 91 contains hardcoded database credentials (
root:root) in theDATABASE_URLenvironment variable. While these are test-only credentials in a disposable CI environment, security tooling flags this pattern and it should be migrated to GitHub Actions secrets.Store credentials in repository secrets and construct the URL dynamically:
env: APP_ENV: ${{ matrix.env || 'test_cached' }} - DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" BEHAT_BASE_CMD: "vendor/bin/behat --colors --strict --no-interaction -vvv -f progress"Then ensure the
DB_USERandDB_PASSWORDsecrets are configured in the repository settings with the test credentials (e.g.,DB_USER=root,DB_PASSWORD=root).
71-78: Add error handling to panther matrix generation.The jq command at line 77 lacks error handling. If
MATRIXcontains invalid JSON, the command will fail silently and an invalid matrix written toGITHUB_OUTPUT.Add explicit error checking:
- name: "Generate panther matrix" id: panther-matrix env: MATRIX: ${{ steps.matrix.outputs.prop }} run: | # Panther tests only run on one PHP version - will be migrated to chromedriver anyway - RESULT=$(jq -cn --argjson matrix "$MATRIX" '{ include: [$matrix.include[0]] }') + RESULT=$(jq -cn --argjson matrix "$MATRIX" '{ include: [$matrix.include[0]] }') || \ + { echo "ERROR: Failed to generate panther matrix. MATRIX=$MATRIX"; exit 1; } echo "matrix=$RESULT" >> $GITHUB_OUTPUT
53-69: Add error handling to chromedriver matrix generation.The jq commands at lines 63-67 lack error handling. If
MATRIXorGROUPSenvironment variables contain invalid JSON, or if the jq transformation fails, the error will be silently ignored and an invalid or empty matrix written toGITHUB_OUTPUT, causing downstream job failures with unclear diagnostics.Add explicit error checking:
- name: "Generate chromedriver matrix" id: chromedriver-matrix env: MATRIX: ${{ steps.matrix.outputs.prop }} GROUPS: ${{ steps.chromedriver-groups.outputs.prop }} BUILD_TYPE: ${{ inputs.type }} run: | # For minimal: use only first PHP/Symfony combo (browser tests don't depend on PHP version) # For full: test all PHP/Symfony combinations if [ "$BUILD_TYPE" == "minimal" ]; then - RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ + RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ '{ include: [ $matrix.include[0] as $m | $groups[] as $g | ($m + { group: $g }) ]}') + [ $? -eq 0 ] || { echo "ERROR: Failed to generate minimal chromedriver matrix. MATRIX=$MATRIX, GROUPS=$GROUPS"; exit 1; } else - RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ + RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ '{ include: [ $matrix.include[] as $m | $groups[] as $g | ($m + { group: $g }) ]}') + [ $? -eq 0 ] || { echo "ERROR: Failed to generate full chromedriver matrix. MATRIX=$MATRIX, GROUPS=$GROUPS"; exit 1; } fi echo "matrix=$RESULT" >> $GITHUB_OUTPUT
🧹 Nitpick comments (1)
.github/workflows/ci_e2e-js.yaml (1)
147-159: Consider adding a guard to detect empty group tags.If
matrix.group.tagswere ever empty or undefined, the for loop at line 153 would not execute any iterations and the job would pass without running any Behat tests. This could silently mask configuration errors inchromedriver-groups.json. While the current groups all have tags defined, adding a defensive check would prevent future configuration mistakes.Add an explicit check before the loop:
- name: Run Behat (Chromedriver - ${{ matrix.group.name }}) env: GROUP_TAGS: ${{ matrix.group.tags }} run: | + if [ -z "$GROUP_TAGS" ]; then + echo "ERROR: GROUP_TAGS is empty for group '${{ matrix.group.name }}'" + exit 1 + fi + # Split group tags by comma and run behat for each IFS=',' read -ra TAGS_ARRAY <<< "$GROUP_TAGS" for TAG in "${TAGS_ARRAY[@]}"; do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__full_2_1.yaml(3 hunks).github/workflows/ci__full_2_2.yaml(3 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-js.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(1 hunks).github/workflows/ci_e2e-mysql.yaml(1 hunks).github/workflows/ci_e2e-pgsql.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci_e2e-mariadb.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/ci_e2e-js.yaml
[medium] 91-92: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
- GitHub Check: Static checks / PHP 8.4, Symfony ~7.3.0
🔇 Additional comments (3)
.github/workflows/ci_e2e-pgsql.yaml (1)
1-1: Workflow naming update is consistent with other renames.The change from "End-to-End (PostgreSQL)" to "Tests (PostgreSQL)" aligns with the workflow renaming strategy across the CI suite and improves clarity by distinguishing these non-JS tests from the new browser-based E2E tests.
.github/workflows/ci_e2e-mysql.yaml (1)
1-1: Workflow refactoring aligns with browser test separation strategy.Renaming from "End-to-End (MySQL)" to "Tests (MySQL)" clarifies that this workflow runs non-browser tests (PHPUnit + Behat API/Domain/UI without JavaScript). The removal of JS browser test jobs to the new ci_e2e-js.yaml workflow cleanly separates concerns and aligns with the PR's parallelization strategy.
.github/workflows/chromedriver-groups.json (1)
1-24: Chromedriver groups configuration is well-structured.The five groups (Products, Catalog & Shipping, Promotions, Checkout, Remaining) organize tests by business domain, enabling parallel execution. The "Remaining" group correctly uses negation logic to ensure comprehensive coverage of tests not explicitly assigned to other groups. This structure supports the PR's goal of reducing ~45 minutes to ~7 minutes per job by parallelizing.
44f74bb to
818d6d4
Compare
818d6d4 to
6733360
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci_e2e-mariadb.yaml (1)
53-62: Clarify Behat tag naming convention.The API path uses environment variables named
BEHAT_NON_UI_*while the UI path usesBEHAT_NON_JS_*(line 172-173). This naming is semantically ambiguous—"NON_UI" suggests the API path excludes UI tests, while "NON_JS" suggests the UI path excludes JavaScript tests. Both are true, but the naming convention makes the distinction unclear. Consider documenting or standardizing these names for clarity, especially if they'll be referenced in other workflows or configuration files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci_e2e-mariadb.yaml(5 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-mysql.yaml.github/workflows/ci_e2e-pgsql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
⏰ 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: Static checks / PHP 8.4, Symfony ~7.3.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
- GitHub Check: Static checks / PHP 8.5, Symfony ~7.4.0
🔇 Additional comments (6)
.github/workflows/ci_e2e-mariadb.yaml (1)
176-187: Inconsistent Node version handling between API and UI paths.The
behat-uijob adds branching for Node version 22.x on branch 2.0 (lines 181-187), but thephpunit-apijob (lines 64-73) only branches on branch 1.14. Verify whether the 2.0 → 22.x mapping should also apply to the API path for consistency..github/workflows/ci_e2e-pgsql.yaml (2)
153-160: Node version branching is inconsistent between API and UI paths.Like the MariaDB workflow, the
phpunit-apijob (lines 63-72) doesn't include the 2.0 → 22.x Node version mapping thatbehat-uiadds (lines 156-158). Ensure both paths handle Node versions consistently across supported branches.
59-60: Database-specific tag handling is correct.The
@no-postgresexclusion tag is properly included in both the API path (line 59) and UI path (line 145), correctly preventing PostgreSQL-incompatible tests from running in both workflows.Also applies to: 145-146
.github/workflows/ci_e2e-mysql.yaml (3)
63-72: Node version branch logic is inconsistent across paths (recurring issue).For the third time across these three workflow files, the
phpunit-apijob (lines 63-72) lacks the branch 2.0 → Node 22.x mapping thatbehat-uiincludes (lines 167-168). This inconsistency appears across all three database backends (MariaDB, PostgreSQL, MySQL). Either both paths need the full branching logic, or neither should have it.Recommended action: Verify whether
phpunit-apigenuinely doesn't need Node 22.x for branch 2.0, or whether allphpunit-apijobs are missing this configuration. If missing, this should be corrected uniformly across all three database workflows.Also applies to: 160-171
84-86: MySQL Twig restriction is properly duplicated in both paths.The "Restrict Twig" step is correctly present in both
phpunit-api(lines 84-86) andbehat-ui(lines 183-185), ensuring consistent Twig version handling regardless of test path. No changes needed here.Also applies to: 183-185
88-98: Manifest.json preparation is consistent across both paths.Both
phpunit-apiandbehat-uijobs include the manifest.json preparation step with identical logic. This is necessary and correct for both paths.Also applies to: 187-197
6733360 to
e189341
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci_e2e-mariadb.yaml (1)
64-73: Node version detection missing branch 2.0 check; inconsistent with behat-ui job.The
phpunit-apijob only checks for branch 1.14, defaulting to NODE_VERSION=24.x. However, thebehat-uijob (lines 176–187) includes a separate check for branch 2.0 → NODE_VERSION=22.x. If the API tests need the same Node version handling, this inconsistency could cause environment mismatches.Update the Set variables step to match the behat-ui logic:
- name: Set variables shell: bash env: BRANCH: ${{ inputs.branch }} run: | if [ "$BRANCH" == "1.14" ]; then echo "NODE_VERSION=20.x" >> $GITHUB_ENV + elif [ "$BRANCH" == "2.0" ]; then + echo "NODE_VERSION=22.x" >> $GITHUB_ENV else echo "NODE_VERSION=24.x" >> $GITHUB_ENV fi
🧹 Nitpick comments (1)
.github/workflows/ci_e2e-mariadb.yaml (1)
91-120: Significant code duplication: manifest preparation and API Platform requirements duplicated in behat-ui job.Lines 91–101 (manifest preparation) and lines 103–120 (API Platform requirements) are repeated verbatim in the
behat-uijob at lines 205–215 and 217–234. Similarly, the BuildTestAppAction configuration (lines 122–134) is nearly identical to the behat-ui version (lines 236–248). This duplication increases maintenance burden and risk of drift.Extract these shared setup steps into a reusable GitHub Actions composite action (e.g.,
.github/actions/setup-e2e-app-mariadb). This would centralize the logic, reduce duplication, and ensure consistency across both jobs. Alternatively, consider using a shared workflow if the matrix and environment context can be passed throughworkflow_call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci__full_1_14.yaml(2 hunks).github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(5 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(3 hunks).github/workflows/ci_static-checks.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci_static-checks.yaml
- .github/workflows/ci__full_1_14.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci__full_2_1.yaml
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mysql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Use PHPStan for static analysis
Applied to files:
.github/workflows/ci__full_2_2.yaml
🔇 Additional comments (3)
.github/workflows/ci_e2e-mariadb.yaml (1)
44-156: Job split and semantic logic look correct.The
phpunit-apijob appropriately runs PHPUnit, CLI Behat, and API Behat (using@api,@domainsuites), while thebehat-uijob handles UI-focused Behat tests (non-JS, excluding chromedriver via~@javascript&&~@mink:chromedriver). This aligns well with the PR objective to move JS/Chromedriver tests toci_e2e-js.yaml. Artifact naming and timeout settings are reasonable..github/workflows/ci_e2e-mysql.yaml (1)
1-223: Well-structured API/UI job split.The refactoring cleanly separates API-focused testing (PHPUnit, CLI, Behat @api/@Domain) from non-JS UI testing, with appropriately scoped Behat tags and environment configurations. The new manifest.json initialization step ensures asset builds work consistently, and node version branching (especially the new 2.0 → 22.x mapping) shows attention to version compatibility.
.github/workflows/ci_e2e-pgsql.yaml (1)
1-210: PostgreSQL workflow properly mirrors MySQL changes with database-specific configs.The PostgreSQL variant correctly applies the API/UI job split with appropriate
@no-postgrestag exclusions, and database-specific URLs and node version handling. Manifest.json initialization and artifact naming are consistent with the MySQL counterpart.
e189341 to
1c69c25
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/ci_js.yaml (2)
91-91: Migrate hardcoded database credentials to GitHub Actions secrets.Lines 91 and 183 contain hardcoded database credentials (
root:root). Never hardcode secrets in code or GitHub Actions workflows; always use the secrets context or environment variables to reference secrets. While these are test-only credentials, this pattern is flagged by security tooling and should be moved to secrets.Apply this diff to reference GitHub Actions secrets:
env: APP_ENV: ${{ matrix.env || 'test_cached' }} - DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" BEHAT_BASE_CMD: "vendor/bin/behat --colors --strict --no-interaction -vvv -f progress"Then configure the
DB_USERandDB_PASSWORDsecrets in the repository settings with the test credentials.Note: This comment was previously flagged; verify that the secrets have been created and the fix applied to both job locations.
Also applies to: 183-183
147-159: Add guard clause to prevent silent pass when GROUP_TAGS is empty.If
matrix.group.tagsis undefined or empty, the for loop executes zero iterations and the job completes successfully without running any Behat tests. This silently masks configuration errors inchromedriver-groups.json.Apply this diff to add a guard check:
- name: Run Behat (Chromedriver - ${{ matrix.group.name }}) env: GROUP_TAGS: ${{ matrix.group.tags }} run: | + if [ -z "$GROUP_TAGS" ]; then + echo "ERROR: GROUP_TAGS is empty for group '${{ matrix.group.name }}'" + exit 1 + fi + # Split group tags by comma and run behat for each IFS=',' read -ra TAGS_ARRAY <<< "$GROUP_TAGS" for TAG in "${TAGS_ARRAY[@]}"; do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__full_1_14.yaml(5 hunks).github/workflows/ci__full_2_1.yaml(4 hunks).github/workflows/ci__full_2_2.yaml(4 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci__unstable.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(0 hunks).github/workflows/ci_js.yaml(7 hunks).github/workflows/ci_mariadb.yaml(1 hunks).github/workflows/ci_mysql.yaml(1 hunks).github/workflows/ci_packages-unstable.yaml(1 hunks).github/workflows/ci_packages.yaml(1 hunks).github/workflows/ci_pgsql.yaml(4 hunks).github/workflows/ci_static-checks.yaml(1 hunks).github/workflows/ci_unstable.yaml(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci_e2e-mariadb.yaml
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci_packages.yaml
- .github/workflows/chromedriver-groups.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci_static-checks.yaml
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Use PHPStan for static analysis
Applied to files:
.github/workflows/ci__full_2_1.yaml.github/workflows/ci__full_1_14.yaml.github/workflows/ci__full_2_2.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_unstable.yaml.github/workflows/ci_pgsql.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_mariadb.yaml
[medium] 55-56: Basic Auth Credentials
(CKV_SECRET_4)
.github/workflows/ci_mysql.yaml
[medium] 55-56: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (21)
- GitHub Check: Javascript Tests (MySQL) / Chromedriver Promotions, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Chromedriver Catalog & Shipping, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Chromedriver Checkout, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Chromedriver Remaining, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Chromedriver Products, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Panther, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Frontend / NodeJS 24.x
- GitHub Check: Tests (MariaDB) / UI, PHP 8.4, Symfony ~6.4.0, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: Tests (MariaDB) / UI, PHP 8.5, Symfony ~7.4.0, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: Tests (MariaDB) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: Tests (MariaDB) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: Packages / PHP 8.4, Symfony ~6.4.0
- GitHub Check: Packages / PHP 8.5, Symfony ~7.3.0
- GitHub Check: Tests (MySQL) / UI, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Tests (MySQL) / UI, PHP 8.5, Symfony ~7.4.0, MySQL 8.4
- GitHub Check: Tests (MySQL) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, MySQL 8.4
- GitHub Check: Tests (MySQL) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Tests (PostgreSQL) / UI, PHP 8.5, Symfony ~7.4.0, PostgreSQL 17.5
- GitHub Check: Tests (PostgreSQL) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, PostgreSQL 15.13
- GitHub Check: Tests (PostgreSQL) / UI, PHP 8.4, Symfony ~6.4.0, PostgreSQL 15.13
- GitHub Check: Tests (PostgreSQL) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, PostgreSQL 17.5
🔇 Additional comments (38)
.github/workflows/ci_packages-unstable.yaml (1)
24-24: Timeout increase aligns with broader CI adjustments.The increased timeout (25 → 45 minutes) for unstable dependency tests is reasonable and consistent with similar adjustments across CI workflows to accommodate longer-running test scenarios.
.github/workflows/ci__unstable.yaml (2)
19-19: Workflow reference updated correctly.The reference to
ci_unstable.yamlaligns with the broader CI restructuring to consolidate and rename workflow files.
23-24: Input type added to packages-unstable job.The new
type: minimalinput is consistent with the matrix-driven workflow patterns introduced elsewhere in this PR..github/workflows/ci_unstable.yaml (5)
19-19: Job naming and structure are clear.The unstable job name now includes all matrix variables for better visibility, making it easier to identify which combination is running.
37-40: Behat tag/suite separation is well-structured.The new BEHAT_API_* and BEHAT_UI_* variables provide clear separation of concerns between API and UI test suites, improving test organization and parallelization.
81-85: Dedicated API and UI Behat steps improve test isolation.Separating API and UI tests into dedicated steps (instead of combining them in a single run) aligns with the PR objective to split test execution for better stability and debuggability.
98-182: Chromedriver job properly isolated.The dedicated
behat-ui-js-chromedriver-unstablejob with separate matrix, environment variables, and rerun strategy (lines 159-169) effectively isolates Chromedriver tests as intended by the PR.
183-260: Panther job maintains consistency.The
behat-ui-js-panther-unstablejob mirrors the Chromedriver structure, ensuring consistent test patterns across different browser drivers..github/workflows/ci__full_1_14.yaml (3)
28-28: Job display name updated for clarity.Renaming to "PHPStan, Lint" more accurately describes the static analysis tools used, improving workflow visibility and CI status reporting.
39-39: Workflow references updated to consolidated files.References to new
ci_mariadb.yaml,ci_mysql.yaml, andci_pgsql.yamlfiles align with the PR's workflow consolidation objective. Ensure these files are in place and correctly configured.Also applies to: 49-49, 59-59
106-109: Slack notification labels updated consistently.The Slack message reflects updated job names ("PHPStan, Lint" and "Tests (…)") for consistency with the new workflow structure.
.github/workflows/ci_pgsql.yaml (6)
1-1: Workflow name improved for clarity.Renaming from "End-to-end tests" to "Tests" aligns with the broader CI nomenclature updates and is more concise.
21-42: Matrix-driven configuration provides flexibility.The get-matrix job reading from
.github/workflows/matrix.jsonenables dynamic test matrix configuration, making it easier to add or modify test combinations without editing the workflow file.Verify that
.github/workflows/matrix.jsoncontains thee2e-pgsqlproperty for all configured build types.
44-130: phpunit-cli-api job properly structures test execution.The separation of PHPUnit, CLI, and API Behat tests into dedicated steps with clear tag filtering improves test organization. The artifact upload on failure provides good debuggability.
118-119: API Behat tests properly isolated.The dedicated "Run API Behat" step with specific tag filtering (
BEHAT_API_TAGSandBEHAT_API_SUITES) correctly separates API tests from CLI tests, aligning with the PR's goal to split test execution.
132-200: New behat-ui job successfully separates UI tests.The dedicated
behat-uijob runs UI-specific tests with filters that exclude JavaScript and Chromedriver tests (~@javascript&&~@mink:chromedriver), leaving browser-based tests to be handled separately. This supports the PR objective of parallel test execution.
55-55: DATABASE_URL contains hardcoded test credentials.The DATABASE_URL on line 55 (and line 143 in behat-ui) contains hardcoded credentials (
postgres:postgres). While this is a standard test environment pattern and likely acceptable, verify that:
- These credentials match the default test setup
- No sensitive production credentials are ever used here
- This pattern is consistent with other database workflow files (MySQL, MariaDB)
.github/workflows/ci_mysql.yaml (6)
1-15: New MySQL workflow follows established patterns.The workflow structure mirrors
ci_pgsql.yaml, providing consistency across database-specific test workflows and making maintenance easier.
21-42: Matrix-driven MySQL configuration aligns with PostgreSQL approach.The get-matrix job enables flexible test matrix management via
matrix.json, consistent with the PostgreSQL workflow.Confirm
.github/workflows/matrix.jsoncontains thee2e-mysqlproperty with appropriate test combinations.
54-60: Environment and tag variables provide clear test separation.The split of API and CLI tests with distinct environment variables follows the established pattern and enables clearer test organization.
84-86: Twig version flexibility is a thoughtful addition.The conditional "Restrict Twig" step allows the matrix to test against different Twig versions, improving test coverage for compatibility concerns. Verify that the matrix.json includes a
twigproperty when needed.
118-119: API and UI Behat steps properly isolated.The separation of API and UI tests into dedicated steps with appropriate tag filtering follows the established pattern and supports the PR's goal of splitting test execution for better parallelization.
Also applies to: 129-130
55-55: DATABASE_URL contains hardcoded test credentials (security flag).Lines 55 and 154 contain the DATABASE_URL with hardcoded credentials (
root:root). While standard for test environments, verify:
- These are only used in test environments
- Consistency with other database workflow files
- No accidental use of production credentials
Also applies to: 154-154
.github/workflows/ci__full_2_1.yaml (5)
28-28: Job naming consolidated for clarity.Renaming static checks to "PHPStan, Lint" and end-to-end tests to "Tests (…)" improves CI workflow visibility and consistency across all similar workflows.
Also applies to: 37-37, 47-47, 57-57
39-39: Workflow references updated and standardized.Database-specific workflows now use the consolidated
ci_mariadb.yaml,ci_mysql.yaml, andci_pgsql.yamlfiles with consistent branch/type inputs, enabling future matrix-driven enhancements.Also applies to: 49-49, 59-62
63-72: New e2e-js job successfully integrated.The
e2e-jsjob is properly configured with:
- Correct workflow reference to
./.github/workflows/ci_js.yaml- Proper dependency on
static-checks- Matrix configuration for the 2.1 branch
- Consistent type: full input
This addresses the PR objective to split Chromedriver tests into parallel execution.
Verify that
./.github/workflows/ci_js.yamlexists and correctly implements the Javascript test execution.
93-93: Notification dependencies now include e2e-js.The
notify-about-build-statusjob now correctly depends one2e-jsalongside other end-to-end jobs, ensuring the notification waits for all tests to complete.
116-120: Slack notification includes e2e-js status.The notification message now reports the result of the Javascript Tests job, providing visibility into browser-based test execution alongside other test categories.
.github/workflows/ci_mariadb.yaml (7)
1-15: New MariaDB workflow provides comprehensive test coverage.The new workflow mirrors the structure of
ci_mysql.yamlandci_pgsql.yamlwhile adding specialized handling for state machine adapters and API Platform versions, addressing MariaDB-specific testing requirements.
44-51: Matrix-driven configuration enables flexible MariaDB test scenarios.The get-matrix job reads from
matrix.json, allowing centralized management of test combinations including PHP, Symfony, MariaDB, state machine adapter, and API Platform versions.Confirm
.github/workflows/matrix.jsonincludes thee2e-mariadbproperty with all required matrix variables:php,symfony,mariadb,state_machine_adapter,api_platform, andtwig.
47-47: Job name template provides excellent visibility.The comprehensive job name includes all matrix variables (state machine adapter, API Platform version), making it easy to identify which test combination is running and quickly locate relevant logs.
85-120: Conditional dependency installation is well-structured.The conditional installation of Winzou State Machine (lines 85-89) and API Platform packages (lines 103-120) correctly handles matrix-driven variations:
- Winzou installed only when
state_machine_adapter == 'winzou_state_machine'- API Platform packages installed only when the matrix specifies a version
This design enables testing multiple state machine and API Platform variants without cluttering the workflow.
118-119: API and UI Behat tests properly separated.The dedicated "Run API Behat" step (lines 118-119) and UI-only behat-ui job (lines 158-262) with appropriate tag filtering enable clear separation of test concerns and support parallel execution.
Also applies to: 144-145
55-55: DATABASE_URL contains hardcoded test credentials.Lines 55 and 169 contain DATABASE_URL with hardcoded credentials (
root:rootfor MariaDB). Verify:
- These credentials match the default MariaDB test container setup
- Consistency with other database workflows
- No production credentials are ever used
Also applies to: 169-169
126-127: Cache keys incorporate multiple matrix variables.The cache keys include
apip-${{ matrix.api_platform || 'none' }}-to differentiate builds with different API Platform versions, preventing cache collision issues when testing multiple API Platform versions against the same code.Also applies to: 240-241
.github/workflows/ci__full_2_2.yaml (3)
63-72: Proper integration of e2e-js job into CI pipeline.The e2e-js job is correctly structured with proper strategy matrix, dependencies, and workflow reference. The job will execute after static-checks and use the ci_js.yaml workflow with appropriate inputs.
93-93: e2e-js correctly added to build status notification dependencies.The e2e-js job is now included in the
notify-about-build-statusneeds array, ensuring the notifier waits for it to complete before reporting status.
116-121: Slack notification properly reflects e2e-js status.The notification text now includes the e2e-js status check alongside other job statuses, providing visibility into browser test results. Job naming and messaging are consistent with other test jobs.
1c69c25 to
2c1a068
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci_js.yaml (1)
63-69: Add error handling to jq matrix generation to prevent silent failures.The jq commands generating chromedriver and panther matrices (lines 63–69, 77–78) lack error detection. If
MATRIXorGROUPScontain invalid JSON or jq fails, the error is silently swallowed and invalid output is written to$GITHUB_OUTPUT, causing confusing downstream job failures.Add error handling to both matrix generation steps:
if [ "$BUILD_TYPE" == "minimal" ]; then - RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ - '{ include: [ $matrix.include[0] as $m | $groups[] as $g | ($m + { group: $g }) ]}') + RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ + '{ include: [ $matrix.include[0] as $m | $groups[] as $g | ($m + { group: $g }) ]}') || \ + { echo "ERROR: Failed to generate minimal chromedriver matrix. MATRIX=$MATRIX, GROUPS=$GROUPS"; exit 1; } else - RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ - '{ include: [ $matrix.include[] as $m | $groups[] as $g | ($m + { group: $g }) ]}') + RESULT=$(jq -cn --argjson matrix "$MATRIX" --argjson groups "$GROUPS" \ + '{ include: [ $matrix.include[] as $m | $groups[] as $g | ($m + { group: $g }) ]}') || \ + { echo "ERROR: Failed to generate full chromedriver matrix. MATRIX=$MATRIX, GROUPS=$GROUPS"; exit 1; } fi echo "matrix=$RESULT" >> $GITHUB_OUTPUTApply similar error handling to the panther matrix generation at lines 77–78:
- RESULT=$(jq -cn --argjson matrix "$MATRIX" '{ include: [$matrix.include[0]] }') + RESULT=$(jq -cn --argjson matrix "$MATRIX" '{ include: [$matrix.include[0]] }') || \ + { echo "ERROR: Failed to generate panther matrix. MATRIX=$MATRIX"; exit 1; } echo "matrix=$RESULT" >> $GITHUB_OUTPUT
🧹 Nitpick comments (1)
.github/workflows/ci_mariadb.yaml (1)
44-156: API and UI job separation is well-structured, but duplicates setup logic.The split of the previous monolithic job into phpunit-cli-api and behat-ui is a clean architectural improvement. However, both jobs repeat nearly identical setup steps (Node versioning, Checkout, Winzou state machine, manifest.json preparation, API Platform packages, and app build). Consider extracting these into a reusable workflow or shared action to reduce duplication and maintenance burden.
Also applies to: 158-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/chromedriver-groups.json(1 hunks).github/workflows/ci__full_1_14.yaml(5 hunks).github/workflows/ci__full_2_1.yaml(4 hunks).github/workflows/ci__full_2_2.yaml(4 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci__unstable.yaml(1 hunks).github/workflows/ci_js.yaml(5 hunks).github/workflows/ci_mariadb.yaml(6 hunks).github/workflows/ci_mysql.yaml(1 hunks).github/workflows/ci_packages-unstable.yaml(1 hunks).github/workflows/ci_packages.yaml(1 hunks).github/workflows/ci_pgsql.yaml(4 hunks).github/workflows/ci_static-checks.yaml(1 hunks).github/workflows/ci_unstable.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/ci__unstable.yaml
- .github/workflows/ci_unstable.yaml
- .github/workflows/ci__full_2_1.yaml
- .github/workflows/ci_pgsql.yaml
- .github/workflows/ci_packages.yaml
- .github/workflows/ci_packages-unstable.yaml
- .github/workflows/ci_static-checks.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Use PHPStan for static analysis
Applied to files:
.github/workflows/ci__full_1_14.yaml.github/workflows/ci__full_2_2.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_js.yaml.github/workflows/ci_mariadb.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_mysql.yaml
[medium] 55-56: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (2)
- GitHub Check: PHPStan, Lint / PHP 8.4, Symfony ~7.3.0
- GitHub Check: PHPStan, Lint / PHP 8.3, Symfony ~6.4.0
🔇 Additional comments (11)
.github/workflows/chromedriver-groups.json (1)
1-24: Well-structured test grouping configuration.The chromedriver-groups.json file provides a clear, modular approach to test parallelization by splitting scenarios into meaningful groups (Products, Catalog & Shipping, Promotions, Checkout, and a Remaining catch-all). The tag syntax aligns with Behat conventions, and the negation logic for the Remaining group ensures comprehensive coverage.
.github/workflows/ci_mariadb.yaml (1)
55-55: Migrate hardcoded database credentials to GitHub Actions secrets.The
DATABASE_URLenvironment variables contain hardcoded credentials (root:root). While these are test-only credentials in a disposable CI environment, security tooling flags this pattern and it should be migrated to GitHub Actions secrets to remove credentials from version control history and CI logs.Store database credentials in repository secrets and construct the URL dynamically:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}"Ensure the
DB_USERandDB_PASSWORDsecrets are configured in the repository settings with the test credentials.Also applies to: 169-169
.github/workflows/ci__full_1_14.yaml (1)
28-28: Workflow reference updates and job label consistency look good.The renaming of static checks and updating workflow references from
ci_e2e-*.yamltoci_*.yamlis consistent and clear. The Slack notification labels correctly reflect the new naming scheme. This 1.14 workflow appropriately maintains a minimal test matrix without the e2e-js job, which aligns with branch-specific CI requirements.Also applies to: 39-39, 49-49, 59-59, 106-110
.github/workflows/ci_mysql.yaml (3)
55-55: Migrate hardcoded database credentials to GitHub Actions secrets.The
DATABASE_URLenvironment variables contain hardcoded credentials (root:root). While these are test-only credentials in a disposable CI environment, security tooling flags this pattern and it should be migrated to GitHub Actions secrets to remove credentials from version control history and CI logs.Store database credentials in repository secrets and construct the URL dynamically:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Ensure the
DB_USERandDB_PASSWORDsecrets are configured in the repository settings with the test credentials.Also applies to: 154-154
112-116: Verify if behat-ui job needs permission fix for Symfony logs.The phpunit-cli-api job includes a "Fix permissions for Symfony logs directory" step (lines 112-116), but the behat-ui job does not. Confirm whether this step is necessary for both jobs or if it's only required after PHPUnit runs. If both jobs write to the logs directory, the permission fix should be applied to behat-ui as well.
100-111: Unable to verify build application step parameters without repository access.The repository could not be accessed to confirm whether ci_mariadb.yaml contains the
databaseandchrome_versionparameters at the referenced lines, or whether ci_mysql.yaml is genuinely missing them. Manual verification against the actual workflow files is required to determine:
- Whether these parameters exist in ci_mariadb.yaml at lines 129-135
- Whether they are missing from ci_mysql.yaml at lines 100-111 and 197-208
- Whether the SyliusLabs/BuildTestAppAction requires these parameters or provides sensible defaults
.github/workflows/ci__minimal.yaml (1)
26-26: Consistent workflow references and proper e2e-js integration.The workflow reference updates (from ci_e2e-.yaml to ci_.yaml) and job name consistency improvements are well-executed. The new e2e-js job is properly integrated with the correct inputs (type: minimal), and all dependent jobs maintain the required input parameters. The naming now clearly reflects the test category (Tests vs. Javascript Tests).
Also applies to: 31-41, 42-47, 49-51
.github/workflows/ci__full_2_2.yaml (1)
63-72: e2e-js job properly integrated into notification workflow.The new e2e-js job is correctly added to the workflow, the notify-about-build-status job includes it in the dependencies array (line 93), and the Slack notification includes its result (line 120). This ensures that JavaScript test failures are properly reported and don't go unnoticed. The integration is consistent with the other end-to-end jobs.
Also applies to: 93-93, 120-120
.github/workflows/ci_js.yaml (3)
91-91: Migrate hardcoded database credentials to GitHub Actions secrets.The
DATABASE_URLenvironment variables in both behat-chromedriver and behat-panther jobs contain hardcoded credentials (root:root). While these are test-only credentials in a disposable CI environment, security tooling flags this pattern and it should be migrated to GitHub Actions secrets to remove credentials from version control history and CI logs.Store database credentials in repository secrets and construct the URL dynamically:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Ensure the
DB_USERandDB_PASSWORDsecrets are configured in the repository settings with the test credentials.Also applies to: 186-186
145-162: GROUP_TAGS safety check is well-implemented.The empty GROUP_TAGS validation (lines 149-152) is a good defensive check that prevents silent test passes when group configuration is missing. This ensures that configuration errors in chromedriver-groups.json are caught immediately rather than masking problems.
46-51: Verify chromedriver-groups.json file exists and is properly handled by the action.The workflow uses
notiz-dev/github-action-json-property@releaseto read.github/workflows/chromedriver-groups.json. Confirm that this file exists in the repository and that the action fails clearly if the file is missing, rather than silently passing invalid configuration downstream. Review the workflow logs or add explicit error handling to catch file-not-found scenarios.
9bdd629 to
3578a19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci_e2e-mariadb.yaml (1)
86-121: Critical code duplication for dependency installation across phpunit-cli-api and behat-ui jobs.The "Require Winzou State Machine" (lines 86-90 vs 199-203) and "Require Api Platform packages" (lines 104-121 vs 217-234) steps are duplicated identically across both jobs. This is particularly problematic because:
- Complex composer requirement logic is harder to keep in sync manually
- Future dependency changes must be made in two places (risk of divergence)
- The duplication makes the file longer and harder to review
Extract the dependency installation steps into a composite action (
.github/actions/install-dependencies/action.yaml):# .github/actions/install-dependencies/action.yaml name: Install Dependencies inputs: state_machine_adapter: required: true api_platform: required: false runs: using: composite steps: - name: Require Winzou State Machine if: ${{ inputs.state_machine_adapter == 'winzou_state_machine' }} shell: bash run: | composer require winzou/state-machine:^0.4 --no-update composer require winzou/state-machine-bundle:^0.6 --no-update - name: Require Api Platform packages if: ${{ inputs.api_platform != '' && inputs.api_platform != null }} shell: bash run: | REQ="${{ inputs.api_platform }}" composer require --no-update \ api-platform/doctrine-common:${REQ} \ # ... etcThen reference it from both jobs:
- uses: ./.github/actions/install-dependencies with: state_machine_adapter: ${{ matrix.state_machine_adapter }} api_platform: ${{ matrix.api_platform }}This consolidates logic, reduces maintenance burden, and makes future changes safer.
Also applies to: 199-234
.github/workflows/ci__full_2_2.yaml (1)
39-42: Update workflow file references to match actual filenames.Lines 39, 49, and 59 reference workflow files that do not exist in the repository. The correct filenames are
ci_mariadb.yaml,ci_mysql.yaml, andci_pgsql.yaml(notci_e2e-mariadb.yaml,ci_e2e-mysql.yaml,ci_e2e-pgsql.yaml). Update all three references accordingly.
♻️ Duplicate comments (1)
.github/workflows/ci_e2e-mysql.yaml (1)
55-55:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 55 and 155 embed database credentials (
root:root) in theDATABASE_URLenvironment variable. This pattern is flagged by security tooling (Checkov CKV_SECRET_4) and should be replaced with GitHub Actions secrets.Apply this diff to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Also applies to: 155-155
🧹 Nitpick comments (2)
.github/workflows/ci_e2e-pgsql.yaml (1)
85-109: Significant code duplication between phpunit-cli-api and behat-ui jobs.The "Prepare manifest.json files" section (lines 85-95 vs 172-182) and "Build application" step (lines 97-109 vs 184-196) are duplicated identically/near-identically across both jobs. This duplication increases maintenance burden and makes future updates error-prone (e.g., if build requirements change, both locations must be updated).
Consider extracting the duplicated "Prepare manifest.json" and "Build application" logic into a composite action or reusable workflow step. Alternatively, if the project uses GitHub Actions composite actions, you could create a
prepare-and-build.yamlcomposite and reference it from both jobs:- name: Prepare and build uses: ./.github/actions/prepare-and-build with: branch: ${{ inputs.branch }} php_version: ${{ matrix.php }} symfony_version: ${{ matrix.symfony }} postgres_version: ${{ matrix.postgres }} node_version: ${{ env.NODE_VERSION }}This would centralize the logic, reduce duplication, and make future modifications simpler.
Also applies to: 172-196
.github/workflows/ci_e2e-mariadb.yaml (1)
123-135: Build application step is duplicated between jobs.The "Build application" step is repeated identically in both phpunit-cli-api (lines 123-135) and behat-ui (lines 236-248) jobs. See recommendation in the companion comment about code duplication for dependency installation.
Also applies to: 236-248
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(6 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci__full_2_1.yaml
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-unstable.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-mysql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (31)
- GitHub Check: Packages / PHP 8.4, Symfony ~6.4.0
- GitHub Check: Packages / PHP 8.5, Symfony ~7.3.0
- GitHub Check: Javascript Tests (MySQL) / Address Book, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Remaining, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Accessing Cart, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Taxons, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Shopping Cart, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Promotion Coupons, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Cart Inventory, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Catalog Promotions, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Checkout, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Countries, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Applying Catalog Promotions, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Shipping Methods, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Promotions, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Zones, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Products, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Javascript Tests (MySQL) / Panther, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Tests (MySQL) / UI, PHP 8.5, Symfony ~7.4.0, MySQL 8.4
- GitHub Check: Tests (MySQL) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, MySQL 8.4
- GitHub Check: Tests (MySQL) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Tests (MySQL) / UI, PHP 8.4, Symfony ~6.4.0, MySQL 8.0
- GitHub Check: Tests (MariaDB) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: Tests (MariaDB) / UI, PHP 8.5, Symfony ~7.4.0, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: Tests (MariaDB) / UI, PHP 8.4, Symfony ~6.4.0, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: Tests (MariaDB) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: Tests (PostgreSQL) / UI, PHP 8.5, Symfony ~7.4.0, PostgreSQL 17.5
- GitHub Check: Tests (PostgreSQL) / PHPUnit, CLI, API, PHP 8.4, Symfony ~6.4.0, PostgreSQL 15.13
- GitHub Check: Tests (PostgreSQL) / PHPUnit, CLI, API, PHP 8.5, Symfony ~7.4.0, PostgreSQL 17.5
- GitHub Check: Tests (PostgreSQL) / UI, PHP 8.4, Symfony ~6.4.0, PostgreSQL 15.13
- GitHub Check: Frontend / NodeJS 24.x
🔇 Additional comments (10)
.github/workflows/ci_e2e-unstable.yaml (5)
35-40: This review comment cannot be applied to the current codebase.The file
.github/workflows/ci_e2e-unstable.yamldoes not exist in the current branch (it exists only in theorigin/2.1branch). Additionally, the code snippet shown in the review comment does not match the actual state of the code:
- Review shows:
CLI_TAGS,CLI_SUITE_TAGS,NON_UI_TAGS,NON_UI_SUITE_TAGS,NON_JS_TAGS,NON_JS_SUITE_TAGS(withoutBEHAT_prefix)- Current branch (
.github/workflows/ci_unstable.yaml):BEHAT_CLI_TAGS,BEHAT_CLI_SUITES,BEHAT_API_TAGS,BEHAT_API_SUITES,BEHAT_UI_TAGS,BEHAT_UI_SUITES- origin/2.1 branch (
.github/workflows/ci_e2e-unstable.yaml):BEHAT_CLI_TAGS,BEHAT_CLI_SUITES,BEHAT_NON_UI_TAGS,BEHAT_NON_UI_SUITES,BEHAT_NON_JS_TAGS,BEHAT_NON_JS_SUITESThe snippet in the review does not match any version of the code in the repository.
Likely an incorrect or invalid review comment.
115-117: The file path is incorrect and the @failing tag purpose is already clearly documented in the workflow.The review references
.github/workflows/ci_e2e-unstable.yaml, but the actual file is.github/workflows/ci_unstable.yaml. More importantly, the workflow already clearly answers all three questions:
- @failing marks randomly failing/flaky tests: The step name at line 56 explicitly states "Run Behat (Chromedriver) for randomly failing scenarios (@failing)"
- @failing only runs in unstable jobs: The tag appears only in
ci_unstable.yamland is excluded (~@failing) in stable workflows (ci_mysql.yaml,ci_pgsql.yaml,ci_mariadb.yaml)- continue-on-error: true at line 61: Allows the workflow to pass even when flaky tests fail, since randomly failing tests are expected to occasionally break and don't indicate a real problem
The workflow design is correct: regular tests run with
~@failingexclusion and retries; flaky tests run separately with@failinginclusion, retries, andcontinue-on-error: trueto permit expected failures.
98-117: Correct file path and variable names: File isci_unstable.yaml(notci_e2e-unstable.yaml), and environment variables areBEHAT_SUITES,BEHAT_TAGS,BEHAT_FAILING_TAGS(notSUITE_TAGS,TAGS,FAILING_TAGS).Both
behat-ui-js-chromedriver-unstable(lines 98-117) andbehat-ui-js-panther-unstable(lines 183-202) intentionally useBEHAT_SUITES: "@hybrid,@ui"for parallel test coverage across different drivers. The jobs run the same test suites but with different driver selectors:
- Chromedriver:
BEHAT_TAGS: "@mink:chromedriver&&~@todo&&~@cli&&~@failing"+ separateBEHAT_FAILING_TAGS- Panther:
BEHAT_TAGS: "@javascript&&~@todo&&~@cli"(no failing tag distinction)This is an appropriate pattern for driver parallelization.
Likely an incorrect or invalid review comment.
35-40: File not found:ci_e2e-unstable.yamldoes not exist in the repository.The referenced file should be
.github/workflows/ci_unstable.yaml. Additionally, the variable names in the review do not match the actual workflow configuration:
- Review mentions:
CLI_TAGS,NON_UI_TAGS,NON_JS_TAGS- Actual variables:
BEHAT_CLI_TAGS,BEHAT_API_TAGS,BEHAT_UI_TAGSThe actual tag combinations in
ci_unstable.yamlare correctly scoped and mutually exclusive:
- behat-no-js-unstable: Splits tests into CLI (@cli), API (
@cli), and UI (@javascript~@mink:chromedriver) with no overlap- behat-ui-js-chromedriver-unstable: Tests requiring @mink:chromedriver, excluding @javascript implicitly
- behat-ui-js-panther-unstable: Tests requiring @javascript, mutually exclusive from chromedriver via driver tag
No changes needed to the workflow configuration itself.
Likely an incorrect or invalid review comment.
161-169: File reference is incorrect and variables don't match the actual codebase.The review references
.github/workflows/ci_e2e-unstable.yamlwith variables$TAGS,$SUITE_TAGS, and$FAILING_TAGS, but the actual file is.github/workflows/ci_unstable.yamlusing$BEHAT_TAGS,$BEHAT_SUITES, and$BEHAT_FAILING_TAGS. Additionally, the concern about whether Panther tests should have triple-rerun logic is based on a false premise—Panther tests already use the same triple-rerun strategy (lines 247-249) as Chromedriver tests. The rerun asymmetry is intentional: browser-based tests (Chromedriver and Panther) use triple-rerun (3 attempts), while non-JS tests (CLI, API, UI at lines 79, 82, 85) use single rerun (2 attempts), reflecting a deliberate distinction between browser test flakiness and non-JS test stability.Likely an incorrect or invalid review comment.
.github/workflows/matrix.json (1)
54-77: Matrix configuration is well-structured and comprehensive.The new e2e-js matrix sections properly organize JavaScript tests into 15 granular groups for parallel execution, with appropriate tag filtering (
@mink:chromedriver, per-feature tags, and exclusions). The "Remaining" group logic effectively captures uncategorized tests. The e2e-js-panther section appropriately uses a single minimal configuration. Configuration is consistent with existing e2e-* patterns in the file..github/workflows/ci_js.yaml (1)
118-124: Verify that matrix-driven job execution handles edge cases safely.The behat-js and behat-panther jobs rely on matrix variables (
matrix.group,matrix.tagsfor behat-js) to drive test execution. Past review comments flagged concerns about silent failures if group tags are empty. While the current code shows tag variables being passed to Behat commands, verify that:
- The matrix generation from matrix.json handles invalid/missing JSON gracefully
- Empty or missing
matrix.group.tagswould cause an obvious failure (not a silent pass)- All matrix inputs are validated before being used in Behat command construction
Please confirm that error handling was added to catch malformed matrix configurations early, preventing jobs from completing successfully without running tests.
Also applies to: 205-209
.github/workflows/ci_e2e-pgsql.yaml (1)
1-1: Workflow naming is now consistent across e2e test suites.The workflow rename from "End-to-End (PostgreSQL)" to "Tests (PostgreSQL)" aligns with broader naming updates across the PR (also seen in ci_e2e-mysql.yaml and ci_e2e-mariadb.yaml), making the CI suite more coherent and distinguishing between non-JavaScript UI tests and new JavaScript tests.
.github/workflows/ci_e2e-mysql.yaml (1)
113-120: Verify that phpunit-cli-api-specific setup steps don't apply to behat-ui.The phpunit-cli-api job includes "Fix permissions for Symfony logs directory" (lines 113-117) and "Test installer" (lines 119-120) steps that are not present in the behat-ui job. This may be intentional (e.g., the installer only runs once before CLI/API tests), but please confirm that the behat-ui job doesn't need these steps or if they should be shared between jobs.
.github/workflows/ci__full_2_2.yaml (1)
63-72: e2e-js job properly integrated into full CI workflow.The new e2e-js job is correctly added to the workflow (lines 63-72) with appropriate configuration (branch and type: full). The notify-about-build-status job's dependencies (line 93) and Slack notification (lines 117-120) have been updated to include the e2e-js job results. This ensures the build status reflects JavaScript test results.
Also applies to: 93-93, 117-120
944ee07 to
09e6315
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci_e2e-mysql.yaml (1)
55-55: 🔴 CRITICAL SECURITY ISSUE: Hardcoded database credentials present across multiple workflows.Hardcoded credentials should never be committed to version control; use GitHub Actions secrets instead. The file
ci_mysql.yaml(notci_e2e-mysql.yaml) contains "root:root" credentials at lines 55 and 154 within the DATABASE_URL environment variable. However, this issue extends beyond this file—identical hardcoded credentials appear in at least 9 other workflow files (ci_frontend.yaml, ci_static-checks.yaml, ci_js.yaml, ci_mariadb.yaml, ci_unstable.yaml, and others).While these are test credentials for disposable CI environments, this pattern violates security best practices and may be flagged by compliance scanning tools (e.g., Checkov CKV_SECRET_4). Replace all instances with
${{ secrets.DB_USER }}and${{ secrets.DB_PASSWORD }}, then configure these secrets in repository settings with the test credentials.Example fix for ci_mysql.yaml:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Apply this change systematically across all affected workflows: ci_frontend.yaml, ci_static-checks.yaml, ci_js.yaml, ci_mariadb.yaml, ci_mysql.yaml, ci_unstable.yaml, and others.
🧹 Nitpick comments (3)
src/Sylius/Behat/Context/Hook/BadGatewayContext.php (1)
37-45: Consider accepting AfterStepScope parameter for consistency.The
delayAfterStepmethod lacks anAfterStepScopeparameter, unlikecheckForBadGatewayErroron Line 24. While the hook framework may allow omitting unused parameters, including it improves consistency within the class and follows the pattern established by the existing hook.Apply this diff to align with the existing hook signature:
#[AfterStep] - public function delayAfterStep(): void + public function delayAfterStep(AfterStepScope $scope): void { $delay = (int) (getenv('BEHAT_STEP_DELAY') ?: 0); if ($delay > 0) { usleep($delay * 1000); } }.github/workflows/ci_e2e-mariadb.yaml (1)
159-248: Consider extracting shared setup steps to a reusable workflow.The
behat-uijob duplicates significant setup logic from thephpunit-cli-apijob (lines 64-135): variable setting, checkout, Winzou/API Platform requirements, manifest preparation, and application build. While this duplication ensures job independence, it increases maintenance burden.Consider extracting the common setup steps (lines 178-248) into a reusable composite action or workflow that both jobs can invoke. This would reduce duplication while maintaining the parallel execution benefits.
.github/workflows/ci_e2e-mysql.yaml (1)
89-99: Refactor duplicated manifest.json preparation step.The manifest.json preparation logic is duplicated across both
phpunit-cli-api(lines 89-99) andbehat-ui(lines 187-197) jobs. Since both jobs perform identical setup, consider extracting this into a shared setup job or using a composite action to reduce duplication and improve maintainability.Also applies to: 187-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(6 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Declarestrict_types=1in all PHP files
Add type declarations for all properties, arguments, and return values in PHP
Usefinalfor all classes, except entities and repositories in PHP
Usereadonlyfor immutable services and value objects in PHP
UsecamelCasefor variables and method names in PHP
UseSCREAMING_SNAKE_CASEfor constants in PHP
Use fast returns instead of nesting logic unnecessarily in PHP
Use trailing commas in multi-line arrays and argument lists in PHP
Order array keys alphabetically where applicable in PHP
Use PHPDoc only when necessary (e.g.,@var Collection<ProductInterface>) in PHP
Group class elements in this order: constants, properties, constructor, public methods, protected methods, private methods in PHP
Group getter and setter methods for the same properties together in PHP
Suffix interfaces with Interface and traits with Trait in PHP
Sortuseimports alphabetically and group by type (classes, functions, constants) in PHP
Use modern PHP 8.2+ syntax and features
Do not use deprecated features from PHP, Symfony, or Sylius
Ensure compatibility with Symfony and PHP versions defined incomposer.json
Files:
src/Sylius/Behat/Context/Hook/BadGatewayContext.php
**/*.{php,yml,yaml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in all files (PHP, YAML, XML, Twig, etc.)
Files:
src/Sylius/Behat/Context/Hook/BadGatewayContext.php
**/*.{php,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Follow secure coding practices to prevent XSS, CSRF, injections, auth bypasses, etc.
Files:
src/Sylius/Behat/Context/Hook/BadGatewayContext.php
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-unstable.yaml.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-mysql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.4, Symfony ~7.3.0
- GitHub Check: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
🔇 Additional comments (15)
.github/workflows/matrix.json (2)
54-101: LGTM! Matrix configuration effectively supports parallel test execution.The new
e2e-jsande2e-js-panthermatrix entries split JavaScript tests into logical groups (Products, Catalog, Promotions, Checkout, Remaining), aligning with the PR objective to improve CI stability and enable faster reruns by isolating scenarios.
198-245: LGTM! Full configuration matrix mirrors minimal setup.The full configuration's
e2e-jsande2e-js-panthersections use appropriate PHP 8.4, Symfony 7.4.0, and MySQL 8.4 combinations, maintaining consistency with the minimal configuration's grouping strategy..github/workflows/ci__minimal.yaml (1)
42-47: LGTM! JavaScript tests properly integrated into minimal CI workflow.The new
e2e-jsjob correctly depends onstatic-checksand invokes theci_js.yamlworkflow withtype: minimal, enabling parallel JavaScript test execution as intended by the PR..github/workflows/ci_e2e-mariadb.yaml (1)
58-62: LGTM! Tag variables improve maintainability.The consolidation of Behat tags into
CLI_TAGS,CLI_SUITE_TAGS,API_TAGS, andAPI_SUITE_TAGSvariables makes the configuration more maintainable and easier to update across multiple run steps..github/workflows/ci_e2e-unstable.yaml (1)
35-40: LGTM! Tag variable updates maintain consistency.The renamed tag variables (
CLI_TAGS,NON_UI_TAGS,NON_JS_TAGS, etc.) align with the standardization applied across other workflow files in this PR, improving maintainability..github/workflows/ci_e2e-pgsql.yaml (1)
58-61: LGTM! PostgreSQL-specific tag exclusions properly configured.The addition of
~@no-postgrestoAPI_TAGSandUI_TAGSappropriately excludes PostgreSQL-incompatible tests, while the tag variable consolidation improves maintainability consistent with other workflow updates..github/workflows/ci_js.yaml (2)
118-138: LGTM! Behat execution logic handles grouped tags effectively.The
run_behatfunction intelligently detects whether tags contain full expressions (for the "Remaining" group) versus simple tags, and theBEHAT_STEP_DELAY=5000on reruns integrates well with the delay hook inBadGatewayContext.phpto address flaky scenarios.
56-56: Consider increasing timeout for certain test groups.The 10-minute timeout may be insufficient for larger test groups, particularly the "Remaining" group which contains all uncategorized scenarios. If jobs timeout, consider raising this to 15-20 minutes or making it group-specific via matrix variables.
Monitor initial CI runs to verify that all groups complete within the 10-minute timeout. If timeouts occur, adjust accordingly.
.github/workflows/ci__full_2_1.yaml (1)
63-72: LGTM! JavaScript tests properly integrated into full CI workflow.The new
e2e-jsjob correctly depends onstatic-checks, uses theci_js.yamlworkflow with branch-specific configuration, and is properly included in the notification dependencies (line 93) and Slack message (line 120)..github/workflows/ci__full_2_2.yaml (2)
63-72: ✅ e2e-js job properly integrated into full CI workflow.The new JavaScript e2e job is correctly configured with matrix strategy, proper dependencies, and follows the established patterns for e2e jobs.
93-93: ✅ Past critical issue resolved: e2e-js properly integrated into build notifications.The previous review flagged that
e2e-jswas missing from thenotify-about-build-statusjob dependencies and Slack notification. Both issues are now correctly addressed:
- Line 93:
e2e-jsadded to theneedsarray- Line 120:
e2e-jsstatus included in Slack message with consistent formattingAlso applies to: 117-120
.github/workflows/ci_e2e-mysql.yaml (4)
44-47: ✅ Job refactoring properly isolates CLI/API tests from UI tests.The split of the original
behat-no-jsjob into two focused jobs (phpunit-cli-apiandbehat-ui) is well-structured and aligns with the PR objective of splitting test scenarios into parallel jobs for better isolation and stability.
phpunit-cli-api: Runs PHPUnit, CLI, and API testsbehat-ui: Runs UI tests exclusivelyThis separation should help isolate flaky scenarios and enable faster reruns.
Also applies to: 144-147
56-61: ✅ Behat tag variables properly defined for test categorization.The introduction of
BEHAT_RERUN_CMDand suite-specific tag variables (CLI_TAGS,CLI_SUITE_TAGS,API_TAGS,API_SUITE_TAGS,UI_TAGS,UI_SUITE_TAGS) provides clear categorization of tests and enables targeted retries with more verbose output (-f pretty), improving debuggability.Also applies to: 157-159
127-131: ✅ Behat execution commands properly structured with fallback retry logic.The Behat commands now include proper error handling with fallback retry using
BEHAT_RERUN_CMDfor both CLI (line 128) and API (line 131) test suites. This improves reliability by re-running failed scenarios with more detailed output.
211-212: ✅ UI Behat execution properly configured with dedicated tag variables.The UI Behat step correctly uses the focused
UI_TAGSandUI_SUITE_TAGSvariables, which appropriately exclude JavaScript and Chromedriver tests (moved to the newci_js.yamlworkflow).
09e6315 to
e2800cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci_e2e-unstable.yaml (1)
152-152: Missingdatabase_versionparameter consistency check.The first job uses
database_version: ${{ matrix.mysql }}(line 64), but the second (line 152) and third (line 234) jobs also specify it. Verify these are consistent with the missingdatabase: "mysql"parameter mentioned in the earlier comment.Also applies to: 234-234
.github/workflows/ci_js.yaml (1)
63-63:⚠️ Replace hardcoded database credentials with GitHub Actions secrets.The
DATABASE_URLenvironment variables on lines 63 and 162 embed test credentials (root:root) in plaintext. This pattern is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets to avoid credential exposure in version control and CI logs.Apply this diff for both the
behat-jsandbehat-pantherjobs:- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Then configure
DB_USERandDB_PASSWORDas repository secrets with appropriate test credentials. Note: This pattern exists across all CI workflows and should be standardized repo-wide.
🧹 Nitpick comments (3)
.github/workflows/ci_js.yaml (1)
224-233: Consider adding branch parameter to Panther artifact name for consistency.The Panther job uploads artifacts with generic naming that lacks branch context (compare to behat-js lines 141-150 which include group/PHP/Symfony/MySQL). While the generic approach works, adding
${{ inputs.branch }}to the artifact name would improve traceability alongside${{ github.run_id }}-${{ github.run_number }}..github/workflows/ci_e2e-pgsql.yaml (1)
81-91: Manifest preparation pattern is consistent but duplicated.The
Prepare manifest.json filesstep is duplicated between thephpunit-cli-apiandbehat-uijobs (lines 81-91 and 170-180). While this ensures each job has independent setup, it introduces maintenance overhead. If the manifest files are shared artifacts, consider consolidating into a separate preparation job.Also applies to: 170-180
.github/workflows/ci_e2e-mariadb.yaml (1)
55-55: Consider extracting database credentials to repository secrets for CI consistency.The
DATABASE_URLenvironment variables on lines 55 and 170 use hardcoded test credentials (root:root). While these are test-only fixtures for local CI environments and not production secrets, extracting them to GitHub Actions secrets would align with security scanning expectations and match repository conventions if such secrets are already in use.If implementing, apply to both jobs and configure
DB_USERandDB_PASSWORDas repository secrets:- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}"Note: The same pattern exists in
ci_e2e-mysql.yaml(lines 55, 155) and should be updated consistently.Also applies to: 170-170
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(4 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Sylius/Behat/Context/Hook/BadGatewayContext.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-mysql.yaml.github/workflows/ci_e2e-unstable.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.3, Symfony ~6.4.0
🔇 Additional comments (13)
.github/workflows/ci_e2e-unstable.yaml (3)
157-168: Chromedriver test split and retry strategy is well-structured.The separation of regular and failing (
@failing) scenarios into two steps (lines 157–167) with independent retry logic matches the PR objective to isolate flaky scenarios. Thecontinue-on-error: trueon the failing scenarios (line 168) prevents job failure if those tests continue to fail intermittently.Verify that the
@failingtag is consistently applied in the feature files and that no scenarios are incorrectly tagged.
63-63: > Likely an incorrect or invalid review comment.
74-76: Tag/suite specifications and retry logic are correct.The explicit tag expressions and suite-tags parameters align with Behat 3.x syntax (using the
sylius-labs/suite-tags-extensiondependency for--suite-tagssupport). Tag operators are properly used:
- AND:
&&(e.g.,@cli&&~@todo)- NOT:
~@(e.g.,~@todo)- OR: comma (e.g.,
@api,@domain)All tag combinations are correct:
- CLI:
@cli&&~@todowith@clisuite ✓- Non-UI:
~@todo&&~@cliwith@api,@domainsuites ✓- Non-JS:
~@javascript&&~@mink:chromedriver&&~@todo&&~@cliwith@hybrid,@uisuites ✓- Chromedriver regular:
@mink:chromedriver&&~@todo&&~@cli&&~@failingwith@hybrid,@uisuites ✓- Chromedriver failing:
@mink:chromedriver&&~@todo&&~@cli&&@failingwith@hybrid,@uisuites ✓- Panther:
@javascript&&~@todo&&~@cliwith@hybrid,@uisuites ✓The retry pattern (two
--rerunattempts for 3 total runs) appropriately supports flaky scenario retries..github/workflows/matrix.json (1)
54-101: Matrix entries well-structured for JavaScript test parallelization.The
e2e-jsmatrix entries are properly configured with per-group tag definitions across minimal and full configurations. The "Remaining" group exhaustively excludes other test groups, allowing isolation of less-categorized scenarios.Also applies to: 198-245
.github/workflows/ci__minimal.yaml (1)
31-53: Job naming and structure updates correctly integrated.The renaming convention ("Tests" vs "Javascript Tests") provides clarity and is consistently applied across the workflow. The new
e2e-jsjob is properly positioned and configured..github/workflows/ci_js.yaml (2)
101-118: Build configuration is comprehensive and consistent.The
BuildTestAppActionusage correctly specifies:
e2e_js: "yes"for JavaScript test environment- Database and version parameters matching matrix values
- Node version conditionally set based on branch
The Twig version restriction (lines 101-103) appropriately precedes the build.
119-139: Behat retry logic and tag handling are well-designed.The per-group tag handling correctly distinguishes between simple tags and compound expressions (Remaining group). The three-tier retry strategy with
BEHAT_STEP_DELAY=5000applied only to reruns is a sound resilience pattern that reduces test flakiness while avoiding unnecessary delays on first attempts..github/workflows/ci_e2e-pgsql.yaml (1)
112-120: Behat tag configuration is comprehensive and properly scoped.CLI, API, and UI test paths are cleanly separated with:
- CLI:
@cli&&~@todowith@clisuite-tags- API:
~@todo&&~@cli&&~@no-postgreswith@api,@domainsuite-tags- UI:
~@javascript&&~@mink:chromedriver&&~@todo&&~@cli&&~@no-postgreswith@hybrid,@uisuite-tagsThe
@no-postgresexclusion in API/UI paths appropriately handles PostgreSQL-specific skips..github/workflows/ci_e2e-mariadb.yaml (1)
82-117: Conditional dependency management is properly implemented.The Winzou State Machine and API Platform requirements are correctly guarded with conditionals, and the cache keys appropriately include
api_platformto avoid collisions. The duplication betweenphpunit-cli-apiandbehat-uijobs is acceptable for job isolation.Also applies to: 197-232
.github/workflows/ci__full_2_1.yaml (1)
90-121: Build status notification properly includes e2e-js job.The
notify-about-build-statusjob correctly:
- Depends on
e2e-js(line 93)- Reports
e2e-jsresult in Slack notification (line 120)However, the
frontendjob (line 73) is not included in theneedsarray, which means the notification may complete before frontend tests finish. Consider addingfrontendto the dependency list if it should be waited for before notification.Should the
frontendjob be added tonotify-about-build-statusdependencies, or is it intentionally excluded?.github/workflows/ci__full_2_2.yaml (1)
63-93: E2E-JS integration is consistent with 2.1 workflow structure.The
e2e-jsjob is properly integrated into the 2.2 workflow with branch-specific matrix configuration and is included in thenotify-about-build-statusdependencies and Slack notifications..github/workflows/ci_e2e-mysql.yaml (2)
123-131: Behat test separation is well-organized and comprehensive.The CLI, API, and UI test paths are cleanly separated with complementary tag sets:
- CLI:
@cli&&~@todo- API:
~@todo&&~@cli- UI:
~@javascript&&~@mink:chromedriver&&~@todo&&~@cliThis ensures no test overlap and comprehensive coverage.
Also applies to: 210-213
55-55: Replace hardcoded database credentials with GitHub Actions secrets.The
DATABASE_URLenvironment variables contain test credentials embedded in plaintext. While acceptable for ephemeral test databases, migrating to GitHub Actions secrets (secrets.DB_USERandsecrets.DB_PASSWORD) is a security best practice.Apply this diff for both
phpunit-cli-apiandbehat-uijobs:- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Then configure
DB_USERandDB_PASSWORDas repository secrets.
feeb0fb to
ca2427b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci__full_1_14.yaml (1)
63-69: Missingstrategy.matrixdefinition in thefrontendjob.The
frontendjob references${{ matrix.branch }}on line 68, but the job lacks astrategy.matrixdefinition. This will causematrix.branchto be undefined, resulting in a workflow error. All other jobs in this workflow (e2e-mariadb, e2e-mysql, e2e-pgsql, packages) include a matrix definition.Add the missing matrix definition to be consistent with other jobs:
frontend: name: Frontend needs: static-checks + strategy: + matrix: + branch: ["1.14"] uses: ./.github/workflows/ci_frontend.yaml with: branch: ${{ matrix.branch }} type: full
♻️ Duplicate comments (1)
.github/workflows/ci_js.yaml (1)
63-64:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets. Lines 63 and 146 embed database credentials (root:root) in theDATABASE_URLenvironment variable. This pattern is flagged by Checkov (CKV_SECRET_4) and should be migrated to GitHub Actions secrets even for test-only environments. Credentials should not appear in source control or CI logs.Configure
DB_USERandDB_PASSWORDsecrets in the repository with test credentials, then construct the URL dynamically:- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Apply this change to both the behat-js job (line 63) and behat-panther job (line 146).
Also applies to: 146-147
🧹 Nitpick comments (1)
.github/workflows/matrix.json (1)
89-90: Complex remaining-group filter may become fragile as new test tags are added. The "Remaining" group uses a long negative filter to capture any @mink:chromedriver scenarios not matched by other groups. While this works, consider documenting the naming convention for new feature tags to ensure they're either explicitly added to a group or captured by the Remaining fallback.Also applies to: 233-234
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci__full_1_14.yaml(2 hunks).github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(4 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Sylius/Behat/Context/Hook/BadGatewayContext.php
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-unstable.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mysql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.php : Ensure compatibility with Symfony and PHP versions defined in `composer.json`
Applied to files:
.github/workflows/matrix.json
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
- GitHub Check: Static checks / PHP 8.4, Symfony ~7.3.0
🔇 Additional comments (14)
.github/workflows/ci__full_1_14.yaml (1)
83-83: Approve the updated job dependencies and notification.The changes to include the
frontendjob in theneedslist and the Slack notification are correct and properly reflect the workflow's dependency structure.Also applies to: 110-110
.github/workflows/ci_e2e-unstable.yaml (2)
63-67: Explicit database parameter improves clarity. Adding thedatabase: "mysql"parameter makes the build action's intent explicit rather than relying on defaults.
73-86: Well-structured Behat rerun logic with explicit tag/suite specifications. The replacement of BEHAT_* environment variables with inline tag specifications and rerun fallbacks is a good consolidation. The two-tier retry approach (base command → rerun twice) is consistent with other E2E workflows..github/workflows/matrix.json (1)
54-101: Well-organized test grouping strategy with clear separation of concerns. The five groups (Managing Products, Taxons/Shipping/Countries/Zones, Promotions, Checkout/Cart/Address, and Remaining) provide logical test distribution for parallel execution. This aligns with the PR objective to split Chromedriver tests into smaller parallel jobs..github/workflows/ci_e2e-mariadb.yaml (2)
1-1: Workflow and job names clearly reflect test scope. Renaming from "End-to-End (MariaDB)" to "Tests (MariaDB)" and the job from "behat-no-js" to "phpunit-cli-api" makes the test scope immediately clear. The conditional API Platform requirement handling in the job name is also a good touch.Also applies to: 44-47
159-252: Parallel UI job mirrors CLI/API structure well. The newbehat-uijob duplicates manifest/build setup fromphpunit-cli-apiand uses identical matrix dependencies. This structure ensures UI tests run independently without interfering with CLI/API execution, supporting the parallelization goal..github/workflows/ci_js.yaml (1)
118-122: BEHAT_STEP_DELAY environment variable on retry is an interesting resilience addition. The delay (5000ms = 5 seconds) is added to the rerun commands but not the initial run. This helps with flaky tests by inserting pauses between steps on retry. Ensure this is intentional and documented if it becomes a pattern across workflows..github/workflows/ci__full_2_1.yaml (2)
37-72: Well-integrated e2e-js job with consistent naming and dependency handling. The new Javascript Tests (MySQL) job is properly positioned, uses the new ci_js.yaml workflow with type: full configuration, and is included in the notify-about-build-status dependencies. The naming convention matches the other Test job renames.
93-122: Slack notification correctly includes e2e-js status. The notify-about-build-status job now waits for e2e-js and includes its result in the Slack message, ensuring visibility of JavaScript test results in the build status report..github/workflows/ci__full_2_2.yaml (1)
63-72: Parallel integration of e2e-js for 2.2 branch workflow. The e2e-js job is consistently integrated into the 2.2 full CI workflow with appropriate branch configuration and type: full parameter, matching the 2.1 workflow structure..github/workflows/ci_e2e-pgsql.yaml (2)
1-1: PostgreSQL workflow name and job restructuring align with MySQL/MariaDB pattern. The rename from "End-to-End (PostgreSQL)" to "Tests (PostgreSQL)" and job split into phpunit-cli-api and behat-ui maintains consistency across database-specific workflows.Also applies to: 44-47
198-199: PostgreSQL-specific tag filter (@no-postgres) appropriately excludes database-incompatible scenarios. The UI Behat run excludes @no-postgres scenarios, acknowledging scenarios that don't support PostgreSQL execution..github/workflows/ci_e2e-mysql.yaml (2)
1-1: MySQL workflow name and job restructuring maintain consistency across E2E workflows. The rename to "Tests (MySQL)" and split into phpunit-cli-api and behat-ui jobs follows the established pattern from MariaDB and PostgreSQL workflows.Also applies to: 44-47
85-95: Manifest preparation and build configuration consistently structured across jobs. Both phpunit-cli-api and behat-ui jobs properly prepare manifest.json files for Symfony 7.4+ compatibility and configure the build with explicit database parameters.Also applies to: 185-195
938b4e6 to
ba872ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci_js.yaml (2)
146-146: ** Hardcoded credentials in behat-panther job.**Line 146 contains the same hardcoded credential pattern as line 63. Apply the same fix using GitHub Actions secrets for consistency.
63-63: Hardcoded database credentials in CI workflow.Lines 63 and 162 contain test credentials (
root:root) embedded directly in theDATABASE_URLenvironment variable. These appear in version control and CI logs in plaintext.Migrate to GitHub Actions secrets:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Create repository secrets
DB_USERandDB_PASSWORDwith test credentials. This removes credentials from source control.
🧹 Nitpick comments (5)
.github/workflows/ci_e2e-mariadb.yaml (1)
159-262: Significant code duplication betweenphpunit-cli-apiandbehat-uijobs.The
behat-uijob (lines 159–262) duplicates ~100 lines ofphpunit-cli-apilogic (checkout, manifest prep, Composer cache, build application, and upload logs steps). The only substantive difference is the Behat tags at line 250.Consider using GitHub Actions workflow templates or composite actions to reduce duplication across this file and similar patterns in
ci_e2e-mysql.yamlandci_e2e-pgsql.yaml..github/workflows/ci_js.yaml (3)
118-122: BEHAT_STEP_DELAY provides stability on retries; consider consistency across jobs.The behat-js run step (lines 120–122) includes
BEHAT_STEP_DELAY=5000on rerun attempts (lines 121–122), which slows down test execution on retry to improve stability by allowing async resources to settle. This is a good practice for browser tests. However, the behat-panther job (lines 203–205) does not include this delay, which may cause similar timing-sensitive flakiness.Consider adding
BEHAT_STEP_DELAY=5000to the panther job reruns for consistency:- name: Run Behat (Panther) run: | $BEHAT_BASE_CMD --tags="@javascript&&~@todo&&~@cli" --suite-tags="@hybrid,@ui" || \ - $BEHAT_RERUN_CMD --tags="@javascript&&~@todo&&~@cli" --suite-tags="@hybrid,@ui" --rerun || \ - $BEHAT_RERUN_CMD --tags="@javascript&&~@todo&&~@cli" --suite-tags="@hybrid,@ui" --rerun + BEHAT_STEP_DELAY=5000 $BEHAT_RERUN_CMD --tags="@javascript&&~@todo&&~@cli" --suite-tags="@hybrid,@ui" --rerun || \ + BEHAT_STEP_DELAY=5000 $BEHAT_RERUN_CMD --tags="@javascript&&~@todo&&~@cli" --suite-tags="@hybrid,@ui" --rerun
187-199: Adddatabase: "mysql"parameter to panther build step for clarity.Line 195 in the behat-panther build step is missing the
database: "mysql"parameter, which is present in the behat-js build step (line 112). While the action may infer the database type from thedatabase_versionparameter, explicitly including it improves clarity and consistency.Apply this diff:
- name: Build application uses: SyliusLabs/BuildTestAppAction@v3.0.1 with: build_type: "sylius" cache_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" cache_restore_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" e2e: "yes" e2e_js: "yes" + database: "mysql" database_version: ${{ matrix.mysql }}This was noted in a past review; please confirm if it requires implementation.
52-133: Substantial code duplication between behat-js and behat-panther jobs.Both jobs (lines 52–133 and 135–216) share nearly identical structure: checkout, cache, Twig restriction, build application, and upload logs. The primary differences are:
- Job names and matrix output variables (lines 55, 138)
- Behat tags (lines 120, 203)
- BEHAT_STEP_DELAY presence (line 121 vs. absent at line 204)
Refactoring this duplication into a reusable composite action or leveraging GitHub Actions'
includematrix feature could improve maintainability and reduce the risk of divergent fixes.Also applies to: 135-216
.github/workflows/ci_e2e-pgsql.yaml (1)
133-210: Code duplication mirrors ci_e2e-mariadb.yaml pattern.The behat-ui job (lines 133–210) duplicates ~75 lines of phpunit-cli-api logic, with the primary difference being Behat tags (line 198 includes @no-postgres exclusion, consistent with the API job). The same optional refactor suggestion for ci_e2e-mariadb.yaml applies here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci__full_1_14.yaml(2 hunks).github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(4 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Sylius/Behat/Context/Hook/BadGatewayContext.php
- .github/workflows/ci__full_1_14.yaml
- .github/workflows/ci_e2e-mysql.yaml
- .github/workflows/ci__minimal.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-unstable.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mariadb.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.php : Ensure compatibility with Symfony and PHP versions defined in `composer.json`
Applied to files:
.github/workflows/matrix.json
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (6)
- GitHub Check: Frontend / Get matrix
- GitHub Check: Tests (PostgreSQL) / Get matrix
- GitHub Check: Packages / Get matrix
- GitHub Check: Tests (MariaDB) / Get matrix
- GitHub Check: Javascript Tests (MySQL) / Get matrix
- GitHub Check: Tests (MySQL) / Get matrix
🔇 Additional comments (18)
.github/workflows/ci_e2e-unstable.yaml (4)
73-86: Inline tag and suite specifications are logically sound.Tag filtering and suite-tags are well-structured to partition test coverage without overlap across the three Behat runs (CLI, non-UI, non-JS). The rerun strategy (2 runs per step) is consistent and appropriate for non-JavaScript tests.
157-168: Chromedriver rerun and flaky-test strategy is sound.Three reruns for Chromedriver tests (vs. two for non-JS tests) and the separate handling of
@failingtests withcontinue-on-error: trueis a well-thought-out approach for managing flaky browser-based tests. The tag partitioning cleanly separates stable and randomly-failing scenarios.
241-245: Panther test configuration is consistent with Chromedriver retry strategy.The three-run rerun logic and tag filtering align with the Chromedriver job's approach for handling flaky JavaScript tests. Note that unlike Chromedriver (lines 163–168), there is no separate step for
@failingscenarios in Panther—this may be intentional if Panther tests are not marked with@failingor are expected to be more stable.Confirm that Panther tests are not tagged with
@failingor that a separate failing-scenario step is not necessary.
56-67: > Likely an incorrect or invalid review comment..github/workflows/ci_e2e-mariadb.yaml (5)
1-1: Workflow rename aligns with PR objectives.Renaming from "End-to-End" to "Tests" improves clarity for the test suite name. The job name split (
behat-no-js→phpunit-cli-api) accurately reflects the job's scope.
44-57: Job name and display changes are clear and appropriate.The expanded display name at line 47 now explicitly mentions PHPUnit, CLI, and API, which aids troubleshooting. BEHAT_BASE_CMD (line 57) and BEHAT_RERUN_CMD (line 58) follow a consistent pattern used across the PR to standardize test rerun behavior.
88-98: Manifest preparation step properly guards against 1.14 branch.The conditional check at line 89 (
if: "${{ inputs.branch != '1.14' }}") ensures manifest files are only prepared when needed. Directory structure and empty JSON initialization are correct.
138-146: Behat run split with rerun logic is well-structured.Separating CLI and API runs into distinct steps with a two-step fallback pattern (base command + rerun) improves test isolation and visibility. The tag and suite filters appropriately exclude @todo and @cli. API run correctly excludes @no-postgres scope.
148-157: Upload logs step correctly targets failures.Artifact naming includes relevant matrix values and run metadata. The conditional
if: failure()ensures logs are only uploaded on test failure, minimizing artifact storage..github/workflows/ci__full_2_1.yaml (3)
37-72: Job naming and new e2e-js job integration are correct.Renamed display names (Lines 37, 47, 57) from "End-to-End" to "Tests" align with the PR's naming convention. The new
e2e-jsjob (lines 63–72) is properly configured with branch matrix, depends onstatic-checks, and references the newci_js.yamlworkflow.
93-93: notify-about-build-status dependencies correctly include e2e-js.The updated
needsarray at line 93 includese2e-jsalongside other jobs, ensuring the notification step waits for all test jobs to complete before reporting.
117-121: Slack notification message includes all job statuses.The updated notification text includes status lines for all jobs, including the new
e2e-js(line 120) andfrontend(line 121) entries, ensuring complete build status visibility..github/workflows/matrix.json (2)
54-101: JavaScript test matrix configuration is well-structured and properly formatted.The new "js" and "js-panther" matrices (lines 54–101 in minimal section) split Chromedriver tests into manageable groups by feature area (Managing Products, Taxons/Shipping, Promotions/Catalog, Checkout/Cart, Remaining). Each group specifies Behat tags with proper @mink:chromedriver isolation and exclusions (@todo, @cli, @failing), enabling parallel execution while preventing test overlap. Tag syntax uses commas and && correctly for Behat filtering.
198-245: Full configuration mirrors minimal with appropriate database versioning.The full configuration (lines 198–245) replicates the minimal JS and JS-panther matrices with MySQL 8.4 (consistent with other full e2e-mysql configurations), ensuring consistent test coverage across build types.
.github/workflows/ci__full_2_2.yaml (2)
37-72: Job naming and e2e-js integration are consistent with ci__full_2_1.yaml.The renamed job display names and new e2e-js job (lines 63–72) follow the same pattern as the 2.1 variant. The integration is correct.
93-93: notify-about-build-status and Slack message properly include e2e-js and frontend.The dependencies (line 93) and notification message (lines 117–121) correctly reflect all jobs, including the new e2e-js and frontend entries.
Also applies to: 117-121
.github/workflows/ci_e2e-pgsql.yaml (2)
1-1: Workflow and job naming follow established pattern.The rename from "End-to-End" to "Tests" and job name split (
behat-no-js→phpunit-cli-api) align with the PR's naming convention and match the pattern in ci_e2e-mariadb.yaml.
44-120: Behat command split and manifest preparation are correctly implemented.The split of CLI and API Behat runs into separate steps (lines 112–120) with base command and rerun logic improves test isolation. API Behat correctly excludes
@no-postgrestags (line 119) to avoid database-specific test failures. Manifest preparation (lines 81–91) properly guards against branch 1.14.
ba872ea to
9eff93f
Compare
9eff93f to
0e7276b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/ci_js.yaml (1)
63-63:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 63 and 146 embed database credentials (
root:root) in theDATABASE_URLenvironment variable. This pattern is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets, even for test-only environments. Credentials should not appear in source control or CI logs.Apply this diff to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Configure
DB_USERandDB_PASSWORDsecrets in the repository with test credentials.Also applies to: 146-146
.github/workflows/ci_e2e-mariadb.yaml (1)
55-55:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 55 and 170 embed database credentials (
root:root) in theDATABASE_URLenvironment variable. This is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets.Apply this diff to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=mariadb-${{ matrix.mariadb }}"Configure
DB_USERandDB_PASSWORDsecrets in the repository with test credentials.Also applies to: 170-170
.github/workflows/ci_e2e-pgsql.yaml (1)
55-55:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 55 and 144 embed database credentials (
postgres:postgres) in theDATABASE_URLenvironment variable. This is flagged by security tooling and should be migrated to GitHub Actions secrets.Apply this diff to both occurrences:
- DATABASE_URL: "pgsql://postgres:postgres@127.0.0.1/sylius?charset=utf8&serverVersion=${{ matrix.postgres }}" + DATABASE_URL: "pgsql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8&serverVersion=${{ matrix.postgres }}"Configure
DB_USERandDB_PASSWORDsecrets (or dedicated PostgreSQL secrets likePG_USERandPG_PASSWORD) in the repository.Also applies to: 144-144
.github/workflows/ci_e2e-mysql.yaml (1)
55-55:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 55 and 155 embed database credentials (
root:root) in theDATABASE_URLenvironment variable. This is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets.Apply this diff to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Configure
DB_USERandDB_PASSWORDsecrets in the repository with test credentials.Also applies to: 155-155
🧹 Nitpick comments (2)
.github/workflows/ci_js.yaml (1)
187-200: Add missingdatabaseparameter to behat-panther build step.The behat-js job (line 112) includes
database: "mysql", but the behat-panther build step (line 195) is missing this parameter. For consistency and to ensure the build action behaves identically, add it here as well.Apply this diff:
- name: Build application uses: SyliusLabs/BuildTestAppAction@v3.0.1 with: build_type: "sylius" cache_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" cache_restore_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" e2e: "yes" e2e_js: "yes" + database: "mysql" database_version: ${{ matrix.mysql }}.github/workflows/ci_e2e-mariadb.yaml (1)
159-252: Consider extracting common job logic between phpunit-cli-api and behat-ui.The behat-ui job (lines 159-252) duplicates approximately 90% of the phpunit-cli-api job structure: identical environment setup, checkout, manifest preparation, build parameters, and similar Behat execution patterns. This creates significant maintenance burden.
GitHub Actions workflows support reusable workflows but not true job composition. However, you could document that behat-ui mirrors phpunit-cli-api with UI-specific tag filtering, or consider whether any shared setup steps could be consolidated in future refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci__full_1_14.yaml(2 hunks).github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(5 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Sylius/Behat/Context/Hook/BadGatewayContext.php
- .github/workflows/ci__minimal.yaml
- .github/workflows/ci_e2e-unstable.yaml
- .github/workflows/ci__full_2_1.yaml
- .github/workflows/ci__full_1_14.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.php : Ensure compatibility with Symfony and PHP versions defined in `composer.json`
Applied to files:
.github/workflows/matrix.json
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mysql.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (6)
.github/workflows/ci_js.yaml (1)
118-122: Verify BEHAT_RERUN_CMD flag placement is correct.Lines 121-122 and 204-205 show the rerun command being constructed differently across workflows. The ci_js.yaml workflow includes
--rerunflag directly in the run step (lines 121-122):BEHAT_RERUN_CMD --tags="${{ matrix.tags }}" --suite-tags="@hybrid,@ui" --rerunHowever, other workflows (ci_e2e-mysql.yaml, ci_e2e-mariadb.yaml) sometimes include
--rerunin the BEHAT_RERUN_CMD environment variable itself. Verify that this placement is intentional and that Behat receives the flag correctly for both behat-js and behat-panther jobs.Also applies to: 201-205
.github/workflows/ci__full_2_2.yaml (1)
63-72: ✓ Properly integrated new e2e-js job into full workflow.The e2e-js job is correctly added to the workflow matrix (lines 63-72), included in the notify-about-build-status dependencies (line 93), and its status is reported in the Slack notification (line 120). This addresses previous critical review comments about missing dependency tracking.
The job naming "Javascript Tests (MySQL)" is consistent with renamed e2e jobs ("Tests (MariaDB)", "Tests (MySQL)", "Tests (PostgreSQL)") and properly triggers the ci_js.yaml workflow with
type: full.Also applies to: 93-93, 120-120
.github/workflows/ci_e2e-pgsql.yaml (1)
57-57: ** Inconsistent BEHAT_RERUN_CMD flag placement across workflows.**In ci_e2e-pgsql.yaml,
BEHAT_RERUN_CMDincludes the--rerunflag directly (line 57):BEHAT_RERUN_CMD: "vendor/bin/behat --colors --strict --no-interaction -vvv -f pretty --rerun"However, in ci_js.yaml, the
--rerunflag is added in the run command itself (lines 121-122), not in the environment variable.Verify that this inconsistency is intentional or standardize the flag placement across all workflow files for clarity and maintainability.
Also applies to: 146-146
.github/workflows/ci_e2e-mysql.yaml (2)
204-204: ✓ Correctly addeddatabaseparameter to behat-ui build step.The behat-ui job (line 204) properly includes
database: "mysql"in the BuildTestAppAction configuration, ensuring consistency with the phpunit-cli-api job. This prevents configuration drift and ensures both jobs use identical build parameters.
123-131: Verify BEHAT_RERUN_CMD consistency: --rerun flag placement varies across workflows.The ci_e2e-mysql.yaml workflow defines
BEHAT_RERUN_CMDwithout the--rerunflag (lines 57, 157), relying on the run command to add it (lines 126, 213):$BEHAT_RERUN_CMD --tags="..." --suite-tags="..." --rerunHowever, ci_e2e-pgsql.yaml includes
--rerunin the BEHAT_RERUN_CMD environment variable itself, while ci_js.yaml also adds it in the run command. This inconsistency across workflows makes maintenance harder.Recommend standardizing this across all e2e workflows. Choose one pattern and apply it consistently:
- Option A: Include
--rerunin BEHAT_RERUN_CMD environment variable (done in ci_e2e-pgsql.yaml)- Option B: Always add
--rerunin the run command (done in ci_js.yaml and ci_e2e-mysql.yaml)Also applies to: 212-213
.github/workflows/matrix.json (1)
54-101: Correct section names and line numbers in the review comment.The review references "js" and "js-panther" sections at lines 54-101 and 198-245, but the actual file contains "e2e-js" and "e2e-js-panther" sections nested under "minimal" and "full" parent keys. These sections do not appear at the specified line numbers.
Regarding Behat tag syntax: the tag filters used in this configuration are valid. Behat supports mixing AND (&&), OR (comma), and NOT (~) operators. The "Remaining" group's complex filter (
@mink:chromedriver&&~@managing_products&&~@managing_taxons...) correctly uses AND with multiple NOT operators, and all referenced tags exist in the codebase.Revise the section names and line number references before resubmitting this review.
Likely an incorrect or invalid review comment.
0e7276b to
c86f287
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/ci_e2e-mysql.yaml (1)
55-55:⚠️ Migrate hardcoded database credentials to GitHub Actions secrets.Lines 55 and 155 embed database credentials (
root:root) in theDATABASE_URLenvironment variable. This pattern is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets. Credentials should never appear in source control or CI logs.Apply these diffs to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" + DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Configure
DB_USERandDB_PASSWORDrepository secrets with test credentials. This same fix should be applied to ci_js.yaml lines 63 and 146, and ci_e2e-unstable.yaml if present.Also applies to: 155-155
.github/workflows/ci_js.yaml (1)
195-199: Adddatabase: "mysql"parameter to behat-panther build step for consistency.Line 112 in the behat-js job includes
database: "mysql", but the behat-panther build step (line 195) is missing this parameter. For consistency and clarity, add it here as well.- name: Build application uses: SyliusLabs/BuildTestAppAction@v3.0.1 with: build_type: "sylius" cache_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" cache_restore_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" e2e: "yes" e2e_js: "yes" + database: "mysql" database_version: ${{ matrix.mysql }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci__full_1_14.yaml(2 hunks).github/workflows/ci__full_2_1.yaml(5 hunks).github/workflows/ci__full_2_2.yaml(5 hunks).github/workflows/ci__minimal.yaml(1 hunks).github/workflows/ci_e2e-mariadb.yaml(6 hunks).github/workflows/ci_e2e-mysql.yaml(6 hunks).github/workflows/ci_e2e-pgsql.yaml(4 hunks).github/workflows/ci_e2e-unstable.yaml(7 hunks).github/workflows/ci_js.yaml(1 hunks).github/workflows/matrix.json(2 hunks)src/Sylius/Behat/Context/Hook/BadGatewayContext.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Sylius/Behat/Context/Hook/BadGatewayContext.php
- .github/workflows/ci__minimal.yaml
- .github/workflows/ci__full_2_2.yaml
- .github/workflows/ci__full_1_14.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.feature : Use Behat for behavior-driven scenarios
Applied to files:
.github/workflows/ci_e2e-unstable.yaml.github/workflows/ci_e2e-pgsql.yaml.github/workflows/ci_e2e-mysql.yaml.github/workflows/ci_e2e-mariadb.yaml
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/*.php : Ensure compatibility with Symfony and PHP versions defined in `composer.json`
Applied to files:
.github/workflows/matrix.json
📚 Learning: 2025-11-25T05:49:22.765Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T05:49:22.765Z
Learning: Applies to **/{admin,shop}/**/*.php : For API configuration, use API Platform 4.x
Applied to files:
.github/workflows/ci_e2e-mariadb.yaml
🪛 Checkov (3.2.334)
.github/workflows/ci_js.yaml
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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: Static checks / PHP 8.5, Symfony ~7.4.0
- GitHub Check: Static checks / PHP 8.4, Symfony ~7.3.0
- GitHub Check: Static checks / PHP 8.3, Symfony ~6.4.0
🔇 Additional comments (7)
.github/workflows/ci_e2e-unstable.yaml (1)
35-35: BEHAT_RERUN_CMD standardization looks good.The refactoring to use
BEHAT_RERUN_CMDenv var is consistent with the broader workflow updates. However, notice that the "Run Behat (Chromedriver)" and "Run Behat (Panther)" steps use a triple-step retry pattern (line 161-163:base || rerun || rerun), while the earlier "Run CLI/non-UI/non-JS Behat" steps use only two steps (lines 75-77, 81-82, 86-87:base || rerun). Verify whether the extra rerun pass for Chromedriver/Panther is intentional or should be standardized to match the two-step pattern used elsewhere.Also applies to: 75-87, 117-117, 161-163, 200-200, 246-248
.github/workflows/ci__full_2_1.yaml (1)
63-72: New e2e-js job properly integrated into orchestration.The
e2e-jsjob is correctly added to the workflow dependency chain (line 93) and included in the Slack notification (line 120), addressing the previous critical issue. Job naming has been consistently updated from "End-to-End" to "Tests" throughout, and the notification properly reports the browser tests status.Also applies to: 93-93, 117-122
.github/workflows/ci_e2e-mariadb.yaml (1)
1-1: Workflow properly refactored with clear job separation.The rename from "End-to-End (MariaDB)" to "Tests (MariaDB)" and the split of jobs (phpunit-cli-api vs behat-ui) provides better clarity and parallelization. The BEHAT_RERUN_CMD pattern is correctly applied, and the Api Platform conditional installation (lines 100-117) appropriately handles matrix variations. The new behat-ui job consolidates non-JS UI tests, aligning with the broader parallelization goal.
Also applies to: 44-44, 47-47, 58-58, 138-147, 159-162
.github/workflows/ci_e2e-pgsql.yaml (1)
1-1: PostgreSQL workflow refactoring is consistent with overall pattern.The rename to "Tests (PostgreSQL)" and job separation into phpunit-cli-api and behat-ui follows the same pattern as MariaDB. The PostgreSQL-specific tag filter (
@no-postgres) is correctly applied to both CLI/API and UI tests, ensuring environment-specific scenarios are skipped appropriately.Also applies to: 44-44, 112-120, 133-200
.github/workflows/ci_e2e-mysql.yaml (1)
144-219: New behat-ui job properly integrated with consistent patterns.The new behat-ui job follows the same refactoring pattern applied to MariaDB and PostgreSQL workflows. Manifest preparation is appropriately added (lines 185-195), and the
database: "mysql"parameter is correctly included (line 204). The Behat execution uses the two-step pattern with BEHAT_RERUN_CMD..github/workflows/ci_js.yaml (1)
38-50: Verify matrix.json provides valid group tags and names for all Behat runs.The workflow reads
jsandjs-panthermatrices frommatrix.jsonusing thenotiz-dev/github-action-json-propertyaction. The behat-js job then usesmatrix.tagsdirectly in Behat commands. Ensure thatmatrix.jsonpopulates thetagsfield with valid, properly-formatted Behat tag expressions for all group entries in bothjsandjs-panthermatrix sections..github/workflows/matrix.json (1)
54-101: The code snippet in this review does not match the actual content at lines 54-101 and 198-245.The review references a code snippet with symfony versions "~7.4.0" and mysql "8.0" simultaneously, which does not exist in the file. The actual minimal.e2e-js section (lines 54-101) uses symfony "~6.4.0" and mysql "8.0", while the full.e2e-js section (lines 198-245) uses symfony "~7.4.0" and mysql "8.4". Additionally, the group names in the review's snippet ("Managing Products", "Taxons, Shipping Methods, Countries, Zones", etc.) differ from the actual group names ("Products", "Catalog", "Promotions", "Checkout", "Remaining").
Regarding tag syntax: the first four groups in each section use comma-separated tags (not AND operators), while only the "Remaining" group uses the && AND logic with negations mentioned in the review. The review's description conflates the two different tag formats.
Likely an incorrect or invalid review comment.
|
|
||
| env: | ||
| APP_ENV: ${{ matrix.env || 'test_cached' }} | ||
| DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}" |
There was a problem hiding this comment.
Lines 63 and 146 embed database credentials (root:root) in the DATABASE_URL environment variable. This pattern is flagged by security tooling (Checkov CKV_SECRET_4) and should be migrated to GitHub Actions secrets, even for test-only environments. Credentials should never appear in source control or CI logs.
Apply these diffs to both occurrences:
- DATABASE_URL: "mysql://root:root@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"
+ DATABASE_URL: "mysql://${{ secrets.DB_USER }}:${{ secrets.DB_PASSWORD }}@127.0.0.1/sylius?charset=utf8mb4&serverVersion=${{ matrix.mysql }}"Configure DB_USER and DB_PASSWORD repository secrets with test credentials, and the URL will be constructed dynamically from secret values.
Also applies to: 146-146
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 63-64: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
.github/workflows/ci_js.yaml around lines 63 and 146: the DATABASE_URL currently
embeds hardcoded credentials ("mysql://root:root@...") which triggers
secret-detection; replace the literal user/password with GitHub Actions secrets
by adding/use environment variables (e.g., DB_USER and DB_PASSWORD) sourced from
secrets (secrets.DB_USER and secrets.DB_PASSWORD) and construct DATABASE_URL
dynamically from those secret-backed vars and the existing host/db/version
parts, and ensure you update the workflow to reference secrets and that these
secrets are configured in the repository settings; also remove any other
occurrences of hardcoded credentials and avoid printing the full URL to logs.
Splits JavaScript browser tests into parallel jobs to avoid timeout issues. **Changes:** - New workflow for JavaScript tests (Chromedriver + Panther) - New `chromedriver-groups.json` with 5 groups: Products, Catalog & Shipping, Promotions, Checkout, Remaining - Updated `ci_e2e-mysql.yaml` - JS tests removed, renamed to "Tests" - Updated `build.yml` to include the new workflow New tests automatically fall into the "Remaining" group. Ref: Sylius/Sylius#18603
Running all scenarios at once makes the build less stable, so splitting them into smaller parts should help us pinpoint which scenarios are flaky. It also means any required reruns will be faster.
The only drawback is that we currently build the same test app in multiple jobs, but the next step is to see if we can build it once and pass it as an artifact to the jobs that need it.
Overall, the build should be faster for anyone running the workflow. The queue might sometimes be a bit longer, but each job will finish sooner, so the total time should still go down.
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.