ci: Minor update for edge case #38295
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow configuration for Appsmith's CI process. The primary changes involve adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Workflow
participant CISystem
participant CypressTests
User->>Workflow: Trigger with run_count
Workflow->>CISystem: Configure test parameters
CISystem->>CypressTests: Run tests (repeat run_count times)
CypressTests-->>CISystem: Return test results
CISystem-->>User: Provide test execution summary
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
|
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 (4)
.github/workflows/ci-test-limited-with-count.yml (4)
Line range hint
12-14: Consider adding maximum limit validation for run_countThe
run_countparameter should have an upper limit to prevent resource exhaustion from excessive test repetitions.run_count: description: 'Number of times to repeat the test run' required: false type: number - default: 1 + default: 1 + max: 5 # Prevent excessive resource usageAlso applies to: 34-36
🧰 Tools
🪛 actionlint (1.7.4)
145-145: shellcheck reported issue in this script: SC2086:info:11:38: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
289-290: Consider using version matrix for Chrome installationHardcoding Chrome version makes maintenance difficult. Consider using a version matrix or environment variable.
- - name: Install Google Chrome 129.0.6668.100 + - name: Install Google Chrome run: | sudo apt-get remove google-chrome-stable - wget -q https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/google-chrome-stable_129.0.6668.100-1_amd64.deb + wget -q https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/google-chrome-stable_${{ env.CHROME_VERSION }}-1_amd64.debAlso applies to: 291-292
🧰 Tools
🪛 actionlint (1.7.4)
145-145: shellcheck reported issue in this script: SC2086:info:11:38: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
471-476: Consolidate artifact upload stepsMultiple similar artifact upload steps could be consolidated into a single step with a matrix of configurations.
Consider using a matrix approach:
- name: Upload artifacts if: always() uses: actions/upload-artifact@v4 strategy: matrix: include: - name: cypress-repeat-logs path: ${{ github.workspace }}/app/client/cy-repeat-summary.txt - name: ci_test_status path: ${{ github.workspace }}/app/client/ci_test_status.txt - name: cypress-console-logs path: ${{ github.workspace }}/app/client/cypress/cypress-logs with: name: ${{ matrix.name }} path: ${{ matrix.path }} overwrite: trueAlso applies to: 478-483, 485-490
🧰 Tools
🪛 actionlint (1.7.4)
145-145: shellcheck reported issue in this script: SC2086:info:11:38: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
315-358: Consider moving sensitive environment variables to GitHub SecretsWhile these variables are masked, consider moving their declarations to repository secrets and referencing them using a centralized configuration.
Create a separate configuration file (e.g.,
.github/config/test-env.yml) and reference it:env: CYPRESS_CREDENTIALS: ${{ secrets.CYPRESS_CREDENTIALS_JSON }}🧰 Tools
🪛 actionlint (1.7.4)
145-145: shellcheck reported issue in this script: SC2086:info:11:38: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-test-limited-with-count.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci-test-limited-with-count.yml (1)
144-144: 🛠️ Refactor suggestion
Simplify the specs validation condition
The current condition is complex and hard to maintain. Consider extracting it into a reusable condition.
- if: ${{ (inputs.specs_to_run == '' || inputs.specs_to_run == null || !inputs.specs_to_run) && steps.run_result.outputs.run_result != 'success' && steps.run_result.outputs.run_result != 'failedtest' }}
+ if: ${{ !inputs.specs_to_run && steps.run_result.outputs.run_result != 'success' && steps.run_result.outputs.run_result != 'failedtest' }}Likely invalid or redundant comment.
## Description Handle one minor edge case. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12431317027> > Commit: 2095f54 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12431317027&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 20 Dec 2024 14:00:30 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a `run_count` parameter for enhanced control over Cypress test execution. - **Bug Fixes** - Improved robustness in checking for null or empty values in the `specs_to_run` input. - **Chores** - Refined logic for rerunning tests based on previous run results and clarified handling of snapshot updates. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Handle one minor edge case.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12431317027
Commit: 2095f54
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 20 Dec 2024 14:00:30 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
run_countparameter for enhanced control over Cypress test execution.Bug Fixes
specs_to_runinput.Chores