-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: visual tests to be enabled #32731
Conversation
WalkthroughThe changes involve updates to Cypress test configurations and scripts within the client application, focusing on enhancing visual test specs and updating text validations. Additionally, a script for uninstalling Node.js is introduced, likely for system cleanup or environment management. Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8720072985. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Actionable comments outside the diff hunks (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
10-10
: Consider adding the article "the" for grammatical correctness:- #ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. + #ci-test-limit uses this file to run the minimum of specs. Do not run the entire suite with this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Actionable comments outside the diff hunks (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
6-6
: Consider adding the article "the" before "minimum of specs" for grammatical correctness.- #ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. + #ci-test-limit uses this file to run the minimum of specs. Do not run entire suite with this command.
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8720744358. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8720744358. |
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8726299542. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Actionable comments outside the diff hunks (1)
app/client/cypress/limited-tests.txt (1)
Line range hint
11-11
: Consider adding the article "the" for grammatical correctness.- #ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command. + #ci-test-limit uses this file to run a minimum of specs. Do not run the entire suite with this command.
/ci-test-limit |
1 similar comment
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8731349601. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8731350026. |
/ci-test-limit |
1 similar comment
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8731657232. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8731657890. |
/ci-test-limit |
@@ -47,7 +47,7 @@ describe("Visual regression tests", { tags: ["@tag.Visual"] }, () => { | |||
cy.get(homePage.signOutIcon).click(); | |||
cy.wait(500); | |||
// validating all the fields on login page | |||
cy.xpath("//h1").should("have.text", "Sign in"); | |||
cy.xpath("//h1").should("have.text", "Sign in to your account"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to pick this value up from the product messages instead of hardcoding this? Using a variable from the product ensures that if this string ever changes, you dont have to update the test separately.
app/client/cypress/limited-tests.txt
Outdated
@@ -1,6 +1,9 @@ | |||
# To run only limited tests - give the spec names in below format: | |||
cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | |||
|
|||
cypress/e2e/Regression/ClientSide/VisualTests/AppPageLayout_spec.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undo all changes from this file. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a small comment. Otherwise this should be good to go.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8749993765. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8749995996. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8749996879. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750000055. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750000870. |
/ci-test-limit runId=8749002405 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8750232063. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
app/client/uninstall-node.sh
Outdated
@@ -0,0 +1,34 @@ | |||
#!/bin/sh | |||
(( ${#} > 0 )) || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace (( ${#} > 0 ))
with a POSIX-compliant check.
- (( ${#} > 0 )) || {
+ [ "$#" -gt 0 ] || {
This change ensures the script is POSIX-compliant and avoids undefined behavior in different shell environments.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
(( ${#} > 0 )) || { | |
[ "$#" -gt 0 ] || { |
app/client/uninstall-node.sh
Outdated
echo 'THE AUTHOR TAKES NO RESPONSIBILITY FOR THE RESULTS OF THIS SCRIPT.' | ||
echo "Disclaimer aside, this worked for the author, for what that's worth." | ||
echo 'Press Control-C to quit now.' | ||
read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use read -r
instead of read
.
- read
+ read -r
The -r
flag prevents backslashes from being interpreted as escape characters, which is important for preserving user input.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
read | |
read -r |
app/client/uninstall-node.sh
Outdated
read | ||
echo 'Re-running the script with sudo.' | ||
echo 'You may be prompted for a password.' | ||
sudo ${0} sudo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enclose ${0}
in double quotes to prevent globbing and word splitting.
- sudo ${0} sudo
+ sudo "${0}" sudo
This change ensures that the script handles paths with spaces or special characters correctly.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
sudo ${0} sudo | |
sudo "${0}" sudo |
app/client/uninstall-node.sh
Outdated
[ -e ${receipt} ] && { | ||
# Loop through all the files in the bom. | ||
lsbom -f -l -s -pf ${receipt} \ | ||
| while read i; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use read -r
in the loop to handle backslashes correctly.
- | while read i; do
+ | while read -r i; do
This modification ensures that file paths containing backslashes are handled correctly.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| while read i; do | |
| while read -r i; do |
app/client/uninstall-node.sh
Outdated
lsbom -f -l -s -pf ${receipt} \ | ||
| while read i; do | ||
# Remove each file listed in the bom. | ||
rm -v /usr/local/${i#/usr/local/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enclose the path in double quotes to prevent globbing and word splitting.
- rm -v /usr/local/${i#/usr/local/}
+ rm -v "/usr/local/${i#/usr/local/}"
This change ensures that the rm
command handles paths with spaces or special characters correctly.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
rm -v /usr/local/${i#/usr/local/} | |
rm -v "/usr/local/${i#/usr/local/}" |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750232063. |
/ci-test-limit runId=8749002405 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8750427513. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750427513. |
/ci-test-limit runId=8749002405 |
1 similar comment
/ci-test-limit runId=8749002405 |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8750597171. |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8750598575. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750598575. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8750597171. |
Enabling visual tests:
RCA:
Whenver there is change in UI , we need recapture the screenshots for each of the tests as mentioned within the spec.
In this exercise we have updated the screenshot for all the visual tests.
Summary by CodeRabbit
New Features
Bug Fixes