test: Remove unused data in DSL fixtures#34405
Merged
Conversation
Contributor
WalkthroughThe changes focus on simplifying JSON configurations used in the Cypress test scenarios. Primarily, the Changes
Poem
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 (
|
sharat87
changed the title
github-actions
bot
added
the
skip-changelog
sharat87
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Event_Bindings_spec.ts (1 hunks)
- app/client/cypress/fixtures/CMSdsl.json (1 hunks)
- app/client/cypress/fixtures/ChartTextDsl.json (1 hunks)
- app/client/cypress/fixtures/Js_toggle_dsl.json (1 hunks)
- app/client/cypress/fixtures/Mapdsl.json (1 hunks)
- app/client/cypress/fixtures/PageLoadDsl.json (1 hunks)
- app/client/cypress/fixtures/PgAdmindsl.json (1 hunks)
- app/client/cypress/fixtures/SimpleBinding.json (1 hunks)
- app/client/cypress/fixtures/TextTableV2dsl.json (1 hunks)
- app/client/cypress/fixtures/TextTabledsl.json (1 hunks)
- app/client/cypress/fixtures/anvilButtonWidget.json (1 hunks)
- app/client/cypress/fixtures/anvilIconButtonWidget.json (1 hunks)
- app/client/cypress/fixtures/anvilInlineButtonWidget.json (1 hunks)
- app/client/cypress/fixtures/anvilToolbarButtonWidget.json (1 hunks)
- app/client/cypress/fixtures/commondsl.json (1 hunks)
- app/client/cypress/fixtures/displayWidgetDsl.json (1 hunks)
- app/client/cypress/fixtures/formdsl.json (1 hunks)
- app/client/cypress/fixtures/layoutdsl.json (1 hunks)
- app/client/cypress/fixtures/publishedModeToastToggleDsl.json (2 hunks)
- app/client/cypress/fixtures/slashcommandDsl.json (1 hunks)
Files skipped from review due to trivial changes (19)
- app/client/cypress/fixtures/CMSdsl.json
- app/client/cypress/fixtures/ChartTextDsl.json
- app/client/cypress/fixtures/Js_toggle_dsl.json
- app/client/cypress/fixtures/Mapdsl.json
- app/client/cypress/fixtures/PageLoadDsl.json
- app/client/cypress/fixtures/PgAdmindsl.json
- app/client/cypress/fixtures/SimpleBinding.json
- app/client/cypress/fixtures/TextTableV2dsl.json
- app/client/cypress/fixtures/TextTabledsl.json
- app/client/cypress/fixtures/anvilButtonWidget.json
- app/client/cypress/fixtures/anvilIconButtonWidget.json
- app/client/cypress/fixtures/anvilInlineButtonWidget.json
- app/client/cypress/fixtures/anvilToolbarButtonWidget.json
- app/client/cypress/fixtures/commondsl.json
- app/client/cypress/fixtures/displayWidgetDsl.json
- app/client/cypress/fixtures/formdsl.json
- app/client/cypress/fixtures/layoutdsl.json
- app/client/cypress/fixtures/publishedModeToastToggleDsl.json
- app/client/cypress/fixtures/slashcommandDsl.json
Additional comments not posted (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Event_Bindings_spec.ts (1)
16-16: Address reported test failures in Rich Text Editor widget tests.The PR description mentions new test failures in several Rich Text Editor widget tests. It's crucial to investigate these failures to ensure they are not caused by the changes made to the DSL files. Review the changes in the affected DSL files and verify if they align with the expected behavior in these tests.
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Event_Bindings_spec.ts
Show resolved
Hide resolved
mohanarpit
approved these changes
Jun 24, 2024
riodeuno
approved these changes
Jun 24, 2024
Contributor
|
@sharat87 Perhaps the RTE test suites are using the removed keys from the fixtures? It seems unlikely, but the tests for the RTE have failed. Let me know if you need help looking into these. |
Member
Author
|
Thanks @riodeuno. Those tests are failing in I'm going to go ahead and merge this PR in soon, since those tests are already failing in the base branch. |
KelvinOm
approved these changes
Jun 24, 2024
sharat87
added a commit
that referenced
this pull request
Jun 26, 2024
Part of #33724, and extension to #34405. This file's contents are used to call the endpoint: ``` /layouts/{layoutId}/pages/{pageId}?applicationId={applicationId} ``` But this endpoint accepts request payload as `LayoutUpdateDTO`, which only has one single field, `dsl`, and nothing else. But the way we use this JSON, is that we're sending the body as this: ```json { "widgetName": "MainContainer", "backgroundColor": "none", "rightColumn": 1296, "snapColumns": 64, "detachFromLayout": true, "widgetId": "0", "topRow": 0, "bottomRow": 440, "containerStyle": "none", "snapRows": 125, "parentRowSpace": 1, "type": "CANVAS_WIDGET", "canExtend": true, "version": 47, "minHeight": 420, "parentColumnSpace": 1, "dynamicBindingPathList": [], "leftColumn": 0, "children": [] } ``` Not as this: ```json { "dsl": { "widgetName": "MainContainer", "backgroundColor": "none", "rightColumn": 1296, "snapColumns": 64, "detachFromLayout": true, "widgetId": "0", "topRow": 0, "bottomRow": 440, "containerStyle": "none", "snapRows": 125, "parentRowSpace": 1, "type": "CANVAS_WIDGET", "canExtend": true, "version": 47, "minHeight": 420, "parentColumnSpace": 1, "dynamicBindingPathList": [], "leftColumn": 0, "children": [] } } ``` Which means that we aren't sending anything useful. /test widget ide <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9680215500> > Commit: 8ede3e7 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9680215500&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget, @tag.IDE` <!-- end of auto-generated comment: Cypress test results -->
sharat87
added a commit
that referenced
this pull request
Jun 27, 2024
Currently, if the incoming request body contains fields that aren't recognized by the backend, we just ignore them, and deserialize what we _can_. This has lead to subtle bugs and wasted a lot of developer time in the past, and added little value in return. Most recently, with application creation, now fixed in #33722. Another one, for action creation, client sends this in `eventData` field of the request body:  But the class defined for `eventData` is this:  Clearly isn't working. This PR enabled the `FAIL_ON_UNKNOWN_PROPERTIES` setting only for the deserialization of HTTP request payloads. It shouldn't have impact on other deserializations like those for Git, and those that load resource files. Primarily to limit the scope. This should also bring in some much-needed type strictness to the client as well. So far, server has been lax in accepting just any fields in the incoming request body, so client was able to afford being lax about the object that was sent up. We're enabling this restriction only for CE currently. Will be opening a similar PR on EE and once all tests pass there, we enable for EE as well. Depends on #33728 #33730 #33731 #34366 #34405 #34446 #34473 #34506 Run No. 1 > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9685619413> > Commit: 04e225f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9685619413&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` **/test all** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9690185000> > Commit: 29da8b4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9690185000&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added `certificateType` property to datasources for enhanced SSL configuration options. - **Enhancements** - Improved JSON deserialization behavior for better data handling and flexibility. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Shivam-z
pushed a commit
to Shivam-z/appsmith
that referenced
this pull request
Jul 10, 2024
Part of appsmithorg#33724. We have a lot of fixture files in our Cypress tests with DSLs of various kinds. They are used to update the DSL using the `/api/v1/layouts/{layoutId}/pages/{pageId}?applicationId={applicationId}`. This API, is [in this line in `LayoutControllerCE`](https://github.com/appsmithorg/appsmith/blob/c4e5d5e6c8f0428fe4a632333b9f6f13de3267b8/app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/LayoutControllerCE.java#L71), and the request body is parsed as [objects of `LayoutUpdateDTO` record](https://github.com/appsmithorg/appsmith/blob/71bc84fc76e85c49312659d18a7632ad62780583/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/LayoutUpdateDTO.java#L6). This `record`, accepts _only_ the `dsl` field, and nothing else. List of potential files that have this problem was identified with this command: ```sh cd app/client/cypress rg -Io 'AddDsl\(".+?\)' \ | cut -d\" -f2 \ | sort -u \ | while read line; do if rg -q '"(layoutOnLoadActions|layoutOnLoadActionErrors|userPermissions|new)":' fixtures/"$line".json; then echo $line fi done ``` (For reference, and to use on EE repo, after this is merged). **/test all** <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > 🔴 🔴 🔴 Some tests have failed. > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9633990737> > Commit: 9e848f1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9633990737&attempt=1&selectiontype=test&testsstatus=failed&specsstatus=fail" target="_blank">Cypress dashboard</a>. > Tags: `` > The following are new failures, please fix them before merging the PR: <ol> > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor3_spec.ts > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_1_spec.js > <li>cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_2_spec.js </ol> > <a href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master" target="_blank">List of identified flaky tests</a>. <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Corrected the file path for loading DSL configuration in the nested list widget scenario. - **Chores** - Streamlined multiple JSON files by removing deprecated `layoutOnLoadActions` arrays and other obsolete fields, resulting in cleaner and more manageable configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Shivam-z
pushed a commit
to Shivam-z/appsmith
that referenced
this pull request
Jul 10, 2024
Part of appsmithorg#33724, and extension to appsmithorg#34405. This file's contents are used to call the endpoint: ``` /layouts/{layoutId}/pages/{pageId}?applicationId={applicationId} ``` But this endpoint accepts request payload as `LayoutUpdateDTO`, which only has one single field, `dsl`, and nothing else. But the way we use this JSON, is that we're sending the body as this: ```json { "widgetName": "MainContainer", "backgroundColor": "none", "rightColumn": 1296, "snapColumns": 64, "detachFromLayout": true, "widgetId": "0", "topRow": 0, "bottomRow": 440, "containerStyle": "none", "snapRows": 125, "parentRowSpace": 1, "type": "CANVAS_WIDGET", "canExtend": true, "version": 47, "minHeight": 420, "parentColumnSpace": 1, "dynamicBindingPathList": [], "leftColumn": 0, "children": [] } ``` Not as this: ```json { "dsl": { "widgetName": "MainContainer", "backgroundColor": "none", "rightColumn": 1296, "snapColumns": 64, "detachFromLayout": true, "widgetId": "0", "topRow": 0, "bottomRow": 440, "containerStyle": "none", "snapRows": 125, "parentRowSpace": 1, "type": "CANVAS_WIDGET", "canExtend": true, "version": 47, "minHeight": 420, "parentColumnSpace": 1, "dynamicBindingPathList": [], "leftColumn": 0, "children": [] } } ``` Which means that we aren't sending anything useful. /test widget ide <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9680215500> > Commit: 8ede3e7 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9680215500&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget, @tag.IDE` <!-- end of auto-generated comment: Cypress test results -->
Shivam-z
pushed a commit
to Shivam-z/appsmith
that referenced
this pull request
Jul 10, 2024
Currently, if the incoming request body contains fields that aren't recognized by the backend, we just ignore them, and deserialize what we _can_. This has lead to subtle bugs and wasted a lot of developer time in the past, and added little value in return. Most recently, with application creation, now fixed in appsmithorg#33722. Another one, for action creation, client sends this in `eventData` field of the request body:  But the class defined for `eventData` is this:  Clearly isn't working. This PR enabled the `FAIL_ON_UNKNOWN_PROPERTIES` setting only for the deserialization of HTTP request payloads. It shouldn't have impact on other deserializations like those for Git, and those that load resource files. Primarily to limit the scope. This should also bring in some much-needed type strictness to the client as well. So far, server has been lax in accepting just any fields in the incoming request body, so client was able to afford being lax about the object that was sent up. We're enabling this restriction only for CE currently. Will be opening a similar PR on EE and once all tests pass there, we enable for EE as well. Depends on appsmithorg#33728 appsmithorg#33730 appsmithorg#33731 appsmithorg#34366 appsmithorg#34405 appsmithorg#34446 appsmithorg#34473 appsmithorg#34506 Run No. 1 > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9685619413> > Commit: 04e225f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9685619413&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` **/test all** <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9690185000> > Commit: 29da8b4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9690185000&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `` <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added `certificateType` property to datasources for enhanced SSL configuration options. - **Enhancements** - Improved JSON deserialization behavior for better data handling and flexibility. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #33724.
We have a lot of fixture files in our Cypress tests with DSLs of various kinds. They are used to update the DSL using the
/api/v1/layouts/{layoutId}/pages/{pageId}?applicationId={applicationId}.This API, is in this line in
LayoutControllerCE, and the request body is parsed as objects ofLayoutUpdateDTOrecord. Thisrecord, accepts only thedslfield, and nothing else.List of potential files that have this problem was identified with this command:
(For reference, and to use on EE repo, after this is merged).
/test all
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9633990737
Commit: 9e848f1
Cypress dashboard.
Tags: ``
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor3_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_1_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/RTE/RichTextEditor_2_spec.js
List of identified flaky tests.Summary by CodeRabbit
Bug Fixes
Chores
layoutOnLoadActionsarrays and other obsolete fields, resulting in cleaner and more manageable configurations.