-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: button group spec #33234
fix: button group spec #33234
Conversation
WalkthroughWalkthroughThe changes primarily focus on standardizing button label handling across various widgets and tests in the codebase. This includes updating default labels, modifying test scripts to use dynamic labels, and ensuring that UI components reflect these changes consistently. Changes
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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/8984051822. |
@@ -163,7 +163,7 @@ describe( | |||
agHelper.ClickButton("Close"); | |||
EditorNavigation.SelectEntityByName("ButtonGroup1", EntityType.Widget); | |||
agHelper.GetNClick(buttongroupwidgetlocators.buttonSettingInPropPane, 0); | |||
propPane.EnterJSContext("onClick", "{{showModal(Modal1.name)}}"); | |||
propPane.EnterJSContext("onClick", "{{showModal('Modal1')}}"); |
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.
@jsartisan Not sure why we are changing this. This was an enhancement done by Aman recently where we can access Modals by using the property Modal.name
instead of Modal1
. If this feature isn't working in Cypress test case, it's a regression & we should resolve this problem in the product rather than in the Cypress test.
CC: @AmanAgarwal041
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.
oh sorry, I didnt take latest pull for release before checking out the new branch. Let me fix this.
agHelper.TypeText("//*[ @value='Do Something']", " New Button"); | ||
agHelper.AssertContains("Do Something New Button"); |
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.
@jsartisan Shouldn't this be "Do something"? Across the entire product, we follow sentence casing. Should the default text on the button follow the same conventions?
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.
Btw, is it possible for us to use a constant variable in the product & Cypress test to avoid such a failure in the future?
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.
The text was suggested by Taras. I'll check with him if we need to change the casing.
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.
Btw, is it possible for us to use a constant variable in the product & Cypress test to avoid such a failure in the future?
Let me check on this.
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 the label in the messages file and used it everywhere. Thanks for the suggestion.
b0d9726
to
26771a5
Compare
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8984186202. |
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
Out of diff range and nitpick comments (1)
app/client/cypress/limited-tests.txt (1)
1-1
: Minor grammatical correction: consider changing "give the spec names in below format" to "give the spec names in the format below".
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8984051822. |
ef6ef09
to
cfea260
Compare
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8984565365. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8984565365. |
/ci-test-limit |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/8985063634. |
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8985063634. |
app/client/cypress/limited-tests.txt
Outdated
@@ -1,5 +1,6 @@ | |||
# 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/Templates/Fork_Template_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.
can we revert this file changes?
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.
Done.
Fixes the button group failing spec
/ok-to-test tags="@tag.Button"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8985593689
Commit: 667b7d1
Cypress dashboard url: Click here!