ci: Update screenshots with CI pipeline#38243
Conversation
WalkthroughThe pull request modifies three GitHub workflow files to enhance the CI process parameterization. New input parameters Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
sagar-qa007
changed the title
|
/ci-test-limit-count run_count=1 update_snapshot=true specs_to_run=cypress/e2e/Regression/ClientSide/Anvil/Widgets/AnvilButtonWidgetSnapshot_spec.ts |
github-actions
bot
added
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci-test-limited-with-count.yml (1)
22-31: LGTM! Input parameters are well-defined.The new parameters
update_snapshotandspecs_to_runare properly configured with appropriate descriptions and default values.Minor formatting: Consider fixing the indentation for the
descriptionfields to match the parent level (8 spaces instead of 10).Also applies to: 48-56
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 23-23: wrong indentation: expected 8 but found 10
(indentation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-client-server-count.yml(5 hunks).github/workflows/ci-test-limited-with-count.yml(5 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-test-limited-with-count.yml
[warning] 23-23: wrong indentation: expected 8 but found 10
(indentation)
[warning] 49-49: wrong indentation: expected 8 but found 10
(indentation)
[error] 133-133: trailing spaces
(trailing-spaces)
[error] 144-144: trailing spaces
(trailing-spaces)
[error] 494-494: trailing spaces
(trailing-spaces)
[error] 501-501: trailing spaces
(trailing-spaces)
.github/workflows/build-client-server-count.yml
[error] 43-43: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/ci-test-limited-with-count.yml (2)
134-140: Snapshot handling implementation is robust.
The implementation properly manages both existing and new snapshots:
- Uploads existing snapshots before tests
- Uploads new snapshots after tests
- Uses
always()condition to ensure snapshots are preserved regardless of test results
Also applies to: 495-501
379-391: Test execution logic is well-implemented.
The conditional handling of snapshot updates and spec selection is clean and effective:
- Clear branching logic for snapshot updates
- Consistent command structure in both branches
- Proper usage of input parameters
.github/workflows/build-client-server-count.yml (2)
20-21: Parameter handling in file-check job is well-implemented.
The implementation:
- Follows consistent patterns for argument parsing
- Sets appropriate default values
- Properly exports values as job outputs
Also applies to: 44-58
170-171: Job input updates are properly implemented.
The changes:
- Consistently pass new parameters to both test jobs
- Properly handle type conversion using fromJson for boolean values
- Maintain workflow structure integrity
Also applies to: 185-186
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/ci-test-limited-with-count.yml (2)
22-31: Fix indentation in workflow inputs.The
update_snapshotinput parameter has incorrect indentation. While functional, it should be consistent with other inputs.Apply this diff to fix the indentation:
update_snapshot: - description: 'Give option to update snapshot (true/false)' - required: false - type: boolean - default: false + description: 'Give option to update snapshot (true/false)' + required: false + type: boolean + default: falseAlso applies to: 48-56
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 23-23: wrong indentation: expected 8 but found 10
(indentation)
133-133: Remove trailing spaces.There are trailing spaces at the end of several lines that should be removed for consistency.
Also applies to: 144-144, 494-494, 501-501
🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-test-limited-with-count.yml(5 hunks).github/workflows/ci-test-limited.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-test-limited-with-count.yml
[warning] 23-23: wrong indentation: expected 8 but found 10
(indentation)
[warning] 49-49: wrong indentation: expected 8 but found 10
(indentation)
[error] 133-133: trailing spaces
(trailing-spaces)
[error] 144-144: trailing spaces
(trailing-spaces)
[error] 494-494: trailing spaces
(trailing-spaces)
[error] 501-501: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/ci-test-limited.yml (1)
421-421: LGTM: Using workspace-relative path for server logs.
Using ${{ github.workspace }} ensures consistent path resolution across different CI environments.
.github/workflows/ci-test-limited-with-count.yml (2)
133-140: LGTM: Improved snapshot comparison workflow.
Good addition of separate steps for uploading existing and new snapshots, enabling better comparison and debugging of visual changes.
Also applies to: 494-501
🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: trailing spaces
(trailing-spaces)
144-144: LGTM: Enhanced spec selection logic.
The condition now properly handles both user-specified specs and default specs from the limited-tests file.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 144-144: trailing spaces
(trailing-spaces)
| if [[ "${{ inputs.update_snapshot }}" == "true" ]]; then | ||
| echo "Running Cypress with snapshot updates..." | ||
| CYPRESS_updateSnapshots=true npx cypress-repeat-pro run -n "${{ inputs.run_count }}" --force \ | ||
| --spec "${{ inputs.specs_to_run }}" \ | ||
| --config-file "cypress_ci_custom.config.ts" \ | ||
| --browser ${{ env.BROWSER_PATH }} | ||
| --browser "${{ env.BROWSER_PATH }}" | ||
| else | ||
| echo "Running Cypress tests without snapshot updates..." | ||
| npx cypress-repeat-pro run -n "${{ inputs.run_count }}" --force \ | ||
| --spec "${{ env.specs_to_run }}" \ | ||
| --config-file "cypress_ci_custom.config.ts" \ | ||
| --browser "${{ env.BROWSER_PATH }}" | ||
| fi |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent spec path usage needs to be fixed
The workflow shows that specs_to_run is set as an environment variable when no specific specs are provided via input. The inconsistency in using inputs.specs_to_run vs env.specs_to_run could lead to test execution issues.
- Line 380: Update mode uses
${{ inputs.specs_to_run }} - Line 388: Non-update mode uses
${{ env.specs_to_run }}
Both modes should consistently use either the input parameter or environment variable to ensure the same specs are run regardless of snapshot update mode.
🔗 Analysis chain
LGTM: Flexible snapshot update handling.
Good implementation of conditional snapshot updates based on the input parameter. The code clearly distinguishes between update and non-update modes.
However, there's a minor inconsistency in spec selection:
- Update mode uses
${{ inputs.specs_to_run }} - Non-update mode uses
${{ env.specs_to_run }}
Let's verify if this difference is intentional:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if specs_to_run is used consistently across the workflow
rg -A 2 'specs_to_run' .github/workflows/ci-test-limited-with-count.yml
Length of output: 1359
1 participant
Description
Adding workflow for updating snapshot with github CI.
Fixes #
Issue NumberAutomation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12412775772
Commit: d77812a
Cypress dashboard.
Tags: @tag.IDE
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Anvil/AnvilAppNavigation_spec.ts
- cypress/e2e/Regression/ClientSide/AppNavigation/AppNavigationWithAutoLayout_spec.ts
- cypress/e2e/Regression/ClientSide/AppNavigation/AppNavigationWithMultiplePages_spec.ts
- cypress/e2e/Regression/ClientSide/ExplorerTests/DragAndDropWidget_spec.js
- cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Property_Pane_1_spec.js
- cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Property_Pane_2_spec.js
- cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Selected_Widgets_spec.js
- cypress/e2e/Regression/ClientSide/IDE/Canvas_View_mode.ts
- cypress/e2e/Regression/ClientSide/IDE/IDE_Add_Pane_Interactions_spec.ts
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Logs1_spec.js
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Logs2_spec.js
List of identified flaky tests.Thu, 19 Dec 2024 13:02:19 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
update_snapshotandspecs_to_runfor enhanced CI workflow configurability.Bug Fixes