DNM: In progress : Fixed chrome version #36972
Conversation
WalkthroughThe pull request introduces significant updates to multiple GitHub Actions workflow files for the Appsmith CI system. Key enhancements include the addition of new input parameters like Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used🔇 Additional comments (4)
🪧 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
the
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: 2
🧹 Outside diff range and nitpick comments (8)
.github/workflows/ci-test-hosted.yml (4)
142-142: Consider using a more flexible Chrome version range.While specifying a exact version ensures consistency, it may require frequent updates. Consider using a version range (e.g.,
^129.0.0) to balance stability with easier maintenance.
Line range hint
228-234: Use GitHub Secrets for sensitive information.Exposing database credentials and access keys in the workflow file poses a security risk. Consider moving these sensitive values to GitHub Secrets and referencing them using the
secretscontext.Example usage:
env: CYPRESS_DB_USER: ${{ secrets.CYPRESS_DB_USER }} CYPRESS_DB_HOST: ${{ secrets.CYPRESS_DB_HOST }} # ... other sensitive variables🧰 Tools
🪛 actionlint
143-143: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
283-301: LGTM. Consider adding a summary of test results.The enhanced Slack notifications with conditional messaging and Cypress Dashboard links are excellent. To further improve, consider including a brief summary of test results (e.g., number of passed/failed tests) in the notification message.
🧰 Tools
🪛 actionlint
143-143: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
314-317: LGTM. Consider using GitHub's built-in status.Saving the run status is a good practice. However, GitHub Actions already provides built-in status for workflow runs. Consider leveraging this feature instead of manually setting the status, unless there's a specific reason for the custom implementation.
🧰 Tools
🪛 actionlint
143-143: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-limited-with-count.yml (3)
Line range hint
18-21: Approved: Addition ofrun_countinput enhances test flexibility.The new
run_countparameter allows for configurable test repetitions, which is useful for identifying flaky tests or ensuring consistency. Consider adding a maximum limit to prevent excessive resource usage.run_count: description: 'Number of times to repeat the test run (max 10)' required: false type: number default: 1 maximum: 10Also applies to: 30-33
🧰 Tools
🪛 actionlint
238-238: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
237-237: Approved: Fixed Chrome version enhances test consistency.Specifying Chrome version 129 ensures consistent browser behavior across test runs. However, consider using an environment variable for easier updates:
chrome-version: ${{ env.CHROME_VERSION }}Then set
CHROME_VERSION: 129in your repository secrets or workflow environment.
Line range hint
334-352: Approved: Cypress test execution now supports configurable repetitions.The implementation of
cypress-repeat-prowith therun_countinput enhances test reliability. The addition of failure detection is a good practice. Consider adding a step to output a summary of all test runs:- name: Output test summary if: always() run: | echo "Test Summary:" cat app/client/cy-repeat-summary.txt🧰 Tools
🪛 actionlint
238-238: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-custom-script.yml (1)
Line range hint
1-30: Consider moving matrix default value to a separate file.The
matrixinput parameter has a long default value, which may become difficult to maintain. Consider moving this to a separate JSON file for better readability and easier updates.🧰 Tools
🪛 actionlint
261-261: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/ci-test-custom-script.yml (1 hunks)
- .github/workflows/ci-test-hosted.yml (1 hunks)
- .github/workflows/ci-test-limited-with-count.yml (1 hunks)
- .github/workflows/ci-test-limited.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
.github/workflows/ci-test-hosted.yml (1)
Line range hint
235-238: LGTM. Please clarify the purpose of CYPRESS_STATIC_ALLOCATION.Using a specific Cypress configuration for CI is a good practice. However, could you provide more information about the purpose and impact of the
CYPRESS_STATIC_ALLOCATIONflag?🧰 Tools
🪛 actionlint
143-143: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-limited-with-count.yml (1)
Line range hint
1-478: Overall: Workflow improvements enhance test flexibility and reliability.The changes to this workflow file significantly improve its functionality:
- The addition of the
run_countparameter allows for more flexible test execution.- Specifying a fixed Chrome version ensures consistent test environments.
- The implementation of test repetition with failure detection enhances the reliability of the CI process.
These modifications collectively contribute to a more robust and configurable CI workflow.
🧰 Tools
🪛 actionlint
238-238: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-custom-script.yml (3)
Line range hint
32-48: LGTM: Job conditions and strategy are well-defined.The refined job conditions and matrix strategy with fail-fast set to false provide good flexibility and ensure comprehensive testing across all matrix jobs.
🧰 Tools
🪛 actionlint
261-261: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
262-385: LGTM: Cypress test setup and execution are well-configured.The extensive list of environment variables and the use of a custom Cypress configuration file provide good test coverage and CI-specific settings.
🧰 Tools
🪛 actionlint
261-261: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
386-486: LGTM: Comprehensive error handling and artifact uploading.The extensive error handling and artifact uploading steps greatly improve debugging capabilities and allow for better management of test reruns.
🧰 Tools
🪛 actionlint
261-261: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
| @@ -224,7 +224,7 @@ jobs: | |||
|
|
|||
| - uses: browser-actions/setup-chrome@latest | |||
| with: | |||
| chrome-version: stable | |||
| chrome-version: 129 | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Chrome version to latest stable
The specified Chrome version (129) is outdated. Update to the latest stable version (130) or use "stable" for automatic updates.
chrome-version: stableThis change ensures compatibility with the current test suite and reduces manual version updates.
🔗 Analysis chain
Verify Chrome version compatibility
The Chrome version has been updated from 'stable' to a specific version '129'. Ensure this version is compatible with our current test suite and CI environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Chrome 129 is the latest stable version
latest_stable=$(curl -s https://omahaproxy.appspot.com/all.json | jq -r '.[] | select(.os=="linux") | .versions[] | select(.channel=="stable") | .version' | head -n1)
if [[ "$latest_stable" == "129."* ]]; then
echo "Chrome 129 is the latest stable version."
else
echo "Warning: Chrome 129 is not the latest stable version. Latest stable is $latest_stable"
fi
Length of output: 405
Script:
#!/bin/bash
# Fetch and display the JSON structure from Omaha Proxy
curl -s https://omahaproxy.appspot.com/all.json | jq .
Length of output: 113
Script:
#!/bin/bash
# Fetch and display the raw JSON structure from Omaha Proxy
curl -s https://omahaproxy.appspot.com/all.json
Length of output: 173
Script:
#!/bin/bash
# Fetch and display the latest stable Chrome version from Chromium Dash
latest_stable=$(curl -s https://chromiumdash.appspot.com/fetch_releases?channel=Stable&platform=Linux | jq -r '.[0].version')
if [[ "$latest_stable" == "129."* ]]; then
echo "Chrome 129 is the latest stable version."
else
echo "Warning: Chrome 129 is not the latest stable version. Latest stable is $latest_stable"
fi
Length of output: 217191
| with: | ||
| chrome-version: stable | ||
| chrome-version: 129 | ||
| - run: | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more flexible Chrome version specification.
Explicitly setting Chrome version to 129 might lead to version conflicts in the future. Consider using a more flexible version specification or regularly updating this value to match the latest stable version.
🧰 Tools
🪛 actionlint
261-261: shellcheck reported issue in this script: SC2086:info:1:40: Double quote to prevent globbing and word splitting
(shellcheck)
| ARG YARN_VERSION='1.22.22' | ||
| ARG NODE_VERSION='20.13.1' | ||
| ARG CYPRESS_VERSION='13.13.0' | ||
| FROM cypress/factory:4.0.2 | ||
| FROM --platform=linux/amd64 cypress/factory:4.0.2 |
There was a problem hiding this comment.
I've tried this configuration, but it doesn't work that way. The docker process starts taking up the entire CPU resource and the tests just hang. I tried it on two different machines(M2 and M3).
There was a problem hiding this comment.
@KelvinOm Yes there is open bug for it apart from this CPU usage. I will spend sometime later once this become priority. cypress-io/cypress#29095
There was a problem hiding this comment.
@sagar-qa007 The issue is that we cannot consider this as a solution. Because we just won't be able to use it. How do you run this locally?You work on a mac like the rest of us, right?
There was a problem hiding this comment.
Yes. I am reiterating this, kindly wait for the fix. I am marking this PR as do not merge as it is in progress.
There was a problem hiding this comment.
As it turns out, the fix of this situation can be waited for a very long time. Can we consider using chromium instead of chrome for testing?
| "cypress:snapshot:docker": "docker run --network='host' -it --rm -e CYPRESS_$1 -v $PWD:/cypress -w /cypress cypress-snapshot $0", | ||
| "cypress:snapshot:docker:build": "docker build . -f cypress/Dockerfile -t cypress-snapshot" | ||
| "cypress:snapshot": "npx cypress run -b chrome -s", | ||
| "cypress:snapshot:docker": "DEBUG=cypress:* docker run --platform linux/amd64 --network='host' -it --rm -e CYPRESS_UPDATE_SNAPSHOTS=$1 -v $PWD:/cypress -w /cypress cypress-snapshot $0", |
There was a problem hiding this comment.
I can't run this script because of error No matches found: "cypress:*".
There was a problem hiding this comment.
I will check this.
github-actions
bot
removed
the
skip-changelog
sagar-qa007
changed the title
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Anvil Snapshot testing required to fix version.
Fixes #
36971Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11475746651
Commit: 50611d6
Cypress dashboard.
Tags: @tag.IDE
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Wed, 23 Oct 2024 08:52:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
ted_tagandrun_count.Improvements
Bug Fixes