-
Notifications
You must be signed in to change notification settings - Fork 4.7k
chore: improved performance of getUnevaluatedDataTree #37189
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
910e7e8
10d158c
92639d5
fdfc5cd
9b53a76
a5b0360
c91d1f7
557172d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +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/Widgets/Datepicker/DatePicker2_spec.js | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 馃挕 Codebase verification Review needed: Test implementation has potential issues The DatePicker2 test file requires attention:
Please:
馃敆 Analysis chainLGTM! Verify the new test implementation. The addition of the DatePicker2 test specification follows the correct format. Let's verify the implementation details of the new test file: 馃弫 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the DatePicker2 test implementation follows best practices
# Test: Check for discouraged practices
echo "Checking for discouraged practices in DatePicker2_spec.js..."
rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Test: Check selector usage
echo "Checking selector patterns..."
rg -i "\.btn\.submit|button\[type=submit\]|\[class=|xpath=|css=" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Test: Verify login/logout implementation
echo "Checking login/logout implementation..."
rg -i "cy\.login|cy\.logout" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
Length of output: 1018 Script: #!/bin/bash
# Let's first find the correct path of the DatePicker2 test file
fd -e js -g "*DatePicker2*spec.js"
# Once we find it, we'll verify its content
Length of output: 120 Script: #!/bin/bash
# Now that we have the correct path, let's check for best practices
echo "Checking for discouraged practices..."
rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
echo "Checking selector patterns..."
rg -i "\.btn\.submit|button\[type=submit\]|\[class=|xpath=|css=" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
echo "Checking login/logout implementation..."
rg -i "cy\.login|cy\.logout" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Let's also check the basic test structure
echo "Checking test structure..."
rg "describe\(|it\(" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
Length of output: 1656 |
||
| # For running all specs - uncomment below: | ||
| #cypress/e2e/**/**/* | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| /* eslint-disable cypress/no-assigning-return-values */ | ||
| /* This file is used to maintain comman methods across tests , refer other *.js files for adding common methods */ | ||
| import { ANVIL_EDITOR_TEST } from "./Constants.js"; | ||
| import advancedFormat from "dayjs/plugin/advancedFormat"; | ||
|
|
||
| import EditorNavigation, { | ||
| EntityType, | ||
|
|
@@ -18,6 +19,7 @@ import { v4 as uuidv4 } from "uuid"; | |
| const dayjs = require("dayjs"); | ||
| const loginPage = require("../locators/LoginPage.json"); | ||
| import homePage from "../locators/HomePage"; | ||
| dayjs.extend(advancedFormat); | ||
|
|
||
| const commonlocators = require("../locators/commonlocators.json"); | ||
| const widgetsPage = require("../locators/Widgets.json"); | ||
|
|
@@ -525,7 +527,7 @@ Cypress.Commands.add("getDate", (date, dateFormate) => { | |
| }); | ||
|
|
||
| Cypress.Commands.add("setDate", (date) => { | ||
| const expDate = dayjs().add(date, "days").format("dddd, MMMM DD"); | ||
| const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 馃挕 Codebase verification Test coverage needed for new date format The existing tests use different date formats (
馃敆 Analysis chainConsider adding a test for the new date format The date format has been updated to include ordinal suffix and year ( Let's verify the test coverage: Consider adding a comment explaining the date format pattern for better maintainability: - const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY");
+ // Format: "Monday, January 1st, 2024"
+ const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY");馃弫 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for tests using setDate command
rg -l "cy.setDate" "app/client/cypress/e2e"
Length of output: 297 Script: #!/bin/bash
# Let's check the actual test content to verify date format coverage
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Binding/DatePicker_Text_spec.js"
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker_With_Switch_spec.js"
# Also search for any existing date format tests
rg "MMMM Do, YYYY" "app/client/cypress/e2e"
Length of output: 2763 |
||
| cy.get(`.react-datepicker__day[aria-label^="Choose ${expDate}"]`).click(); | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,88 +1,71 @@ | ||
| import { generateDataTreeAction } from "ee/entities/DataTree/dataTreeAction"; | ||
| import { generateDataTreeJSAction } from "ee/entities/DataTree/dataTreeJSAction"; | ||
| import { generateDataTreeWidget } from "entities/DataTree/dataTreeWidget"; | ||
| import log from "loglevel"; | ||
| import { | ||
| ENTITY_TYPE, | ||
| EvaluationSubstitutionType, | ||
| } from "ee/entities/DataTree/types"; | ||
| import { generateDataTreeModuleInputs } from "ee/entities/DataTree/utils"; | ||
| import type { | ||
| DataTreeSeed, | ||
| AppsmithEntity, | ||
| EntityTypeValue, | ||
| } from "ee/entities/DataTree/types"; | ||
| import type { | ||
| unEvalAndConfigTree, | ||
| ConfigTree, | ||
| UnEvalTree, | ||
| } from "entities/DataTree/dataTreeTypes"; | ||
| import type { EntityTypeValue } from "ee/entities/DataTree/types"; | ||
| import type { ConfigTree, UnEvalTree } from "entities/DataTree/dataTreeTypes"; | ||
| import { isEmpty } from "lodash"; | ||
| import { generateModuleInstance } from "ee/entities/DataTree/dataTreeModuleInstance"; | ||
| import { | ||
| endSpan, | ||
| startNestedSpan, | ||
| startRootSpan, | ||
| } from "UITelemetry/generateTraces"; | ||
| import { endSpan, startRootSpan } from "UITelemetry/generateTraces"; | ||
| import type { ActionDataState } from "ee/reducers/entityReducers/actionsReducer"; | ||
| import type { JSCollectionDataState } from "ee/reducers/entityReducers/jsActionsReducer"; | ||
| import type { LayoutSystemTypes } from "layoutSystems/types"; | ||
| import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer"; | ||
| import type { MetaState } from "reducers/entityReducers/metaReducer"; | ||
| import type { LoadingEntitiesState } from "reducers/evaluationReducers/loadingEntitiesReducer"; | ||
| import type { MetaWidgetsReduxState } from "reducers/entityReducers/metaWidgetsReducer"; | ||
| import type { Module } from "ee/constants/ModuleConstants"; | ||
| import type { ModuleInstance } from "ee/constants/ModuleInstanceConstants"; | ||
| import type { | ||
| DependencyMap, | ||
| FormEditorConfigs, | ||
| } from "utils/DynamicBindingUtils"; | ||
| export class DataTreeFactory { | ||
| static create({ | ||
| actions, | ||
| appData, | ||
| editorConfigs, | ||
| isMobile, | ||
| jsActions, | ||
| layoutSystemType, | ||
| loadingEntities, | ||
| metaWidgets, | ||
| moduleInputs, | ||
| moduleInstanceEntities, | ||
| moduleInstances, | ||
| pluginDependencyConfig, | ||
| theme, | ||
| widgets, | ||
| widgetsMeta, | ||
| }: DataTreeSeed): unEvalAndConfigTree { | ||
| public static metaWidgets( | ||
| metaWidgets: MetaWidgetsReduxState, | ||
| widgetsMeta: MetaState, | ||
| loadingEntities: LoadingEntitiesState, | ||
| ) { | ||
| const dataTree: UnEvalTree = {}; | ||
| const configTree: ConfigTree = {}; | ||
| const start = performance.now(); | ||
| const startActions = performance.now(); | ||
| const rootSpan = startRootSpan("DataTreeFactory.create"); | ||
| const actionsSpan = startNestedSpan("DataTreeFactory.actions", rootSpan); | ||
| const metaWidgetsSpan = startRootSpan("DataTreeFactory.metaWidgets"); | ||
|
|
||
| actions.forEach((action) => { | ||
| const editorConfig = editorConfigs[action.config.pluginId]; | ||
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; | ||
| const { configEntity, unEvalEntity } = generateDataTreeAction( | ||
| action, | ||
| editorConfig, | ||
| dependencyConfig, | ||
| Object.values(metaWidgets).forEach((widget) => { | ||
| const { configEntity, unEvalEntity } = generateDataTreeWidget( | ||
| widget, | ||
| widgetsMeta[widget.metaWidgetId || widget.widgetId], | ||
| loadingEntities, | ||
| ); | ||
|
|
||
| dataTree[action.config.name] = unEvalEntity; | ||
| configTree[action.config.name] = configEntity; | ||
| }); | ||
| const endActions = performance.now(); | ||
|
|
||
| endSpan(actionsSpan); | ||
|
|
||
| const startJsActions = performance.now(); | ||
| const jsActionsSpan = startNestedSpan( | ||
| "DataTreeFactory.jsActions", | ||
| rootSpan, | ||
| ); | ||
|
|
||
| jsActions.forEach((js) => { | ||
| const { configEntity, unEvalEntity } = generateDataTreeJSAction(js); | ||
|
|
||
| dataTree[js.config.name] = unEvalEntity; | ||
| configTree[js.config.name] = configEntity; | ||
| dataTree[widget.widgetName] = unEvalEntity; | ||
| configTree[widget.widgetName] = configEntity; | ||
| }); | ||
| const endJsActions = performance.now(); | ||
| endSpan(metaWidgetsSpan); | ||
|
|
||
| endSpan(jsActionsSpan); | ||
| return { | ||
| dataTree, | ||
| configTree, | ||
| }; | ||
| } | ||
|
|
||
| const startWidgets = performance.now(); | ||
| const widgetsSpan = startNestedSpan("DataTreeFactory.widgets", rootSpan); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| public static widgets( | ||
| moduleInputs: Module["inputsForm"], | ||
| moduleInstances: Record<string, ModuleInstance> | null, | ||
| moduleInstanceEntities: null, | ||
| widgets: CanvasWidgetsReduxState, | ||
| widgetsMeta: MetaState, | ||
| loadingEntities: LoadingEntitiesState, | ||
| layoutSystemType: LayoutSystemTypes, | ||
| isMobile: boolean, | ||
| ) { | ||
| const dataTree: UnEvalTree = {}; | ||
| const configTree: ConfigTree = {}; | ||
| const widgetsSpan = startRootSpan("DataTreeFactory.widgets"); | ||
|
|
||
| if (!isEmpty(moduleInputs)) { | ||
| const { configEntity, unEvalEntity } = | ||
|
|
@@ -120,54 +103,60 @@ export class DataTreeFactory { | |
| dataTree[widget.widgetName] = unEvalEntity; | ||
| configTree[widget.widgetName] = configEntity; | ||
| }); | ||
|
|
||
| const endWidgets = performance.now(); | ||
|
|
||
| endSpan(widgetsSpan); | ||
|
|
||
| dataTree.appsmith = { | ||
| ...appData, | ||
| // combine both persistent and transient state with the transient state | ||
| // taking precedence in case the key is the same | ||
| store: appData.store, | ||
| theme, | ||
| } as AppsmithEntity; | ||
| (dataTree.appsmith as AppsmithEntity).ENTITY_TYPE = ENTITY_TYPE.APPSMITH; | ||
|
|
||
| const startMetaWidgets = performance.now(); | ||
| const metaWidgetsSpan = startNestedSpan( | ||
| "DataTreeFactory.metaWidgets", | ||
| rootSpan, | ||
| ); | ||
| return { | ||
| dataTree, | ||
| configTree, | ||
| }; | ||
| } | ||
|
|
||
| Object.values(metaWidgets).forEach((widget) => { | ||
| const { configEntity, unEvalEntity } = generateDataTreeWidget( | ||
| widget, | ||
| widgetsMeta[widget.metaWidgetId || widget.widgetId], | ||
| loadingEntities, | ||
| ); | ||
| public static jsActions(jsActions: JSCollectionDataState) { | ||
| const dataTree: UnEvalTree = {}; | ||
| const configTree: ConfigTree = {}; | ||
| const actionsSpan = startRootSpan("DataTreeFactory.jsActions"); | ||
|
|
||
| dataTree[widget.widgetName] = unEvalEntity; | ||
| configTree[widget.widgetName] = configEntity; | ||
| jsActions.forEach((js) => { | ||
| const { configEntity, unEvalEntity } = generateDataTreeJSAction(js); | ||
|
|
||
| dataTree[js.config.name] = unEvalEntity; | ||
| configTree[js.config.name] = configEntity; | ||
| }); | ||
| const endMetaWidgets = performance.now(); | ||
| endSpan(actionsSpan); | ||
|
|
||
| endSpan(metaWidgetsSpan); | ||
| endSpan(rootSpan); | ||
| return { | ||
| dataTree, | ||
| configTree, | ||
| }; | ||
| } | ||
|
|
||
| const end = performance.now(); | ||
| public static actions( | ||
| actions: ActionDataState, | ||
| editorConfigs: FormEditorConfigs, | ||
| pluginDependencyConfig: Record<string, DependencyMap>, | ||
| ) { | ||
| const dataTree: UnEvalTree = {}; | ||
| const configTree: ConfigTree = {}; | ||
| const actionsSpan = startRootSpan("DataTreeFactory.actions"); | ||
|
|
||
| const out = { | ||
| total: end - start, | ||
| widgets: endWidgets - startWidgets, | ||
| actions: endActions - startActions, | ||
| jsActions: endJsActions - startJsActions, | ||
| metaWidgets: endMetaWidgets - startMetaWidgets, | ||
| }; | ||
| actions.forEach((action) => { | ||
| const editorConfig = editorConfigs[action.config.pluginId]; | ||
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; | ||
|
Comment on lines
+143
to
+144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle missing plugin configurations gracefully When accessing |
||
| const { configEntity, unEvalEntity } = generateDataTreeAction( | ||
| action, | ||
| editorConfig, | ||
| dependencyConfig, | ||
| ); | ||
|
|
||
| log.debug("### Create unevalTree timing", out); | ||
| dataTree[action.config.name] = unEvalEntity; | ||
| configTree[action.config.name] = configEntity; | ||
| }); | ||
| endSpan(actionsSpan); | ||
|
|
||
| return { unEvalTree: dataTree, configTree }; | ||
| return { | ||
| dataTree, | ||
| configTree, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Standardize date format usage across assertions.
The date formats are inconsistent between
setDateand the assertion. This could lead to flaky tests.Apply this change to maintain consistency: