Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Improve Linting performance #23865

Merged
merged 65 commits into from
Jul 5, 2023
Merged

feat: Improve Linting performance #23865

merged 65 commits into from
Jul 5, 2023

Conversation

ohansFavour
Copy link
Contributor

@ohansFavour ohansFavour commented May 30, 2023 •
edited
Loading

Description

This PR introduces a new architecture, making evaluation and linting independent.

Screenshot 2023-07-04 at 17 24 40

In the previous architecture, one dependency graph was used to hold the relationship between entities in the application and subsequently, the "evaluation order" and "paths to lint" were generated.

Although similar, the dependency graph required for evaluation and linting differ. For example, trigger fields should not depend on any other entity/entity path in the eval's dependency graph since they are not reactive. This is not the case for the linting dependency graph.

Performance

  • This PR introduces "lint only" actions. These actions trigger linting, but not evaluation. For example, UPDATE_JS_ACTION_BODY_INIT (which is fired immediately after a user edits the body of a JS Object). Since linting fires without waiting for a successful update on the server, response time decreases by 40% (from 2s to 1.2s).

  • Reduction in time taken to generate paths requiring linting.

Screenshot 2023-07-04 at 18 10 52

PR fixes following issue(s)

Fixes #23447
Fixes #23166
Fixes #24194
Fixes #23720
Fixes #23868
Fixes #21895

Latest DP: https://appsmith-r3f9e325p-get-appsmith.vercel.app/

Type of change

  • Chore (housekeeping or task changes that don't impact user perception)

Testing

How Has This Been Tested?

  • Manual
  • Jest
  • Cypress

Test Plan

#23865 (comment)

Issues raised during DP testing

#23865 (comment)
response: #23865 (comment)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Speedbreak features have been covered
  • Test plan covers all impacted features and areas of interest
  • Test plan has been peer reviewed by project stakeholders and other QA members
  • Manually tested functionality on DP
  • We had an implementation alignment call with stakeholders post QA Round 2
  • Cypress test cases have been added and approved by SDET/manual QA
  • Added Test Plan Approved label after Cypress tests were reviewed
  • Added Test Plan Approved label after JUnit tests were reviewed

@ohansFavour ohansFavour marked this pull request as draft May 30, 2023 09:50
@github-actions github-actions bot added the Enhancement New feature or request label May 30, 2023
@ohansFavour ohansFavour changed the title feat: eval-lint split chore: port linting using graph May 30, 2023
@github-actions github-actions bot added skip-changelog Adding this label to a PR prevents it from being listed in the changelog Javascript Product Issues related to users writing javascript in appsmith JS Evaluation Issues related to JS evaluation on the platform JS Linting & Errors Issues related to JS Linting and errors Task A simple Todo and removed Enhancement New feature or request labels May 30, 2023
@ohansFavour ohansFavour force-pushed the feat/separate-linting-from-eval branch from 7d6b741 to bbdeb62 Compare June 13, 2023 00:34
@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5259861401.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5259861401_1

@ohansFavour ohansFavour marked this pull request as ready for review June 13, 2023 22:01
@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5260780344.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5260780344_1

@github-actions
Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5260780344.
Commit: ``.
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/Linting/AsyncFunctionsBoundInSyncFields_Spec.ts

  2. cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
  3. cypress/e2e/Regression/ClientSide/Linting/EntityPropertiesLint_spec.ts
  4. cypress/e2e/Regression/ClientSide/Linting/ErrorReporting_spec.ts
  5. cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js
  6. cypress/e2e/Regression/ServerSide/GenerateCRUD/Mongo_Spec.ts
  7. cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts
To know the list of identified flaky tests - Refer here

@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5264499340.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5264499340_1

@github-actions
Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5260780344.
Commit: ``.
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/ClientSide/Linting/AsyncFunctionsBoundInSyncFields_Spec.ts

  2. cypress/e2e/ClientSide/Linting/BasicLint_spec.ts
  3. cypress/e2e/ClientSide/Linting/EntityPropertiesLint_spec.ts
  4. cypress/e2e/ClientSide/Linting/ErrorReporting_spec.ts
  5. cypress/e2e/ClientSide/Performance/LinkRelPreload_Spec.js
  6. cypress/e2e/ServerSide/QueryPane/Mongo_Spec.ts
To know the list of identified flaky tests - Refer here

@github-actions
Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5264499340.
Commit: ``.
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/Linting/AsyncFunctionsBoundInSyncFields_Spec.ts

  2. cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
  3. cypress/e2e/Regression/ClientSide/Linting/ErrorReporting_spec.ts
  4. cypress/e2e/Regression/ClientSide/MobileResponsiveTests/ConversionFlow_Corner_Cases_spec.ts
  5. cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js
  6. cypress/e2e/Regression/ClientSide/Templates/Fork_Template_To_App_spec.js
To know the list of identified flaky tests - Refer here

@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5271022700.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5271022700_1

@github-actions github-actions bot added Bug Something isn't working Medium Issues that frustrate users due to poor UX and removed Bug Something isn't working labels Jun 14, 2023
@github-actions
Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5271022700.
Commit: ``.
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/Linting/AsyncFunctionsBoundInSyncFields_Spec.ts

  2. cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
  3. cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js
  4. cypress/e2e/Regression/ClientSide/Widgets/Tab/Tab_spec.js
  5. cypress/e2e/Regression/ServerSide/GenerateCRUD/Mongo_Spec.ts
  6. cypress/e2e/Regression/ServerSide/QueryPane/Mongo_Spec.ts
To know the list of identified flaky tests - Refer here

@github-actions github-actions bot added Query & JS Pod Issues related to the query & JS Pod Bug Something isn't working labels Jun 14, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5453197785.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js

  2. cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts
To know the list of identified flaky tests - Refer here

@ohansFavour ohansFavour changed the title feat: port linting using graph feat: Improve Linting performance Jul 4, 2023
@github-actions github-actions bot removed the Bug Something isn't working label Jul 4, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5453197785.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Performance/LinkRelPreload_Spec.js

  2. cypress/e2e/Widgets/TableV2/table_data_change_spec.ts
To know the list of identified flaky tests - Refer here

@ohansFavour ohansFavour force-pushed the feat/separate-linting-from-eval branch from 91e848a to 9e9630b Compare July 4, 2023 15:56
@github-actions github-actions bot added the Bug Something isn't working label Jul 4, 2023
@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5456603669.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5456603669_1

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5456603669.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/Performance/LinkRelPreload_Spec.js

To know the list of identified flaky tests - Refer here

@github-actions github-actions bot added Bug Something isn't working and removed Bug Something isn't working labels Jul 4, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5456603669.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/LinkRelPreload_Spec.js

To know the list of identified flaky tests - Refer here

@github-actions github-actions bot added the Bug Something isn't working label Jul 5, 2023
@ohansFavour
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5464203804.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23865.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23865&runId=5464203804_1

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5464203804.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

@ohansFavour ohansFavour requested a review from arunvjn July 5, 2023 13:24
@ohansFavour ohansFavour merged commit e6f2dca into release Jul 5, 2023
@ohansFavour ohansFavour deleted the feat/separate-linting-from-eval branch July 5, 2023 13:34
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5464203804.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

@ohansFavour ohansFavour mentioned this pull request Jul 6, 2023
Merged
19 tasks
ohansFavour added a commit that referenced this pull request Jul 7, 2023
@ohansFavour
fix:remove linting related code from eval (#24995)
721ea41 
## Description
In #23865 , a new
architecture was introduced that makes linting and evaluation
independent. Sequal to that change, this PR removes redundant linting
code from eval code.

- Removes the triggerfield dependencyMap
- Removes the "lint order" generated from eval flow
- Removes "extraPathsToLint"

#### PR fixes following issue(s)
Fixes #23448

#### Type of change
- Chore (housekeeping or task changes that don't impact user perception)

## Testing
>
#### How Has This Been Tested?
> Please describe the tests that you ran to verify your changes. Also
list any relevant details for your test configuration.
> Delete anything that is not relevant
- [ ] Manual
- [ ] Jest
- [ ] Cypress
>
>
#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed
@iamakulov iamakulov mentioned this pull request Jul 12, 2023
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunvjn arunvjn arunvjn approved these changes

@ramsaptami ramsaptami ramsaptami left review comments

Assignees
No one assigned
Labels
Bug Something isn't working Enhancement New feature or request High This issue blocks a user from building or impacts a lot of users Javascript Product Issues related to users writing javascript in appsmith JS Evaluation Issues related to JS evaluation on the platform JS Linting & Errors Issues related to JS Linting and errors JS Objects Issues related to JS Objects Medium Issues that frustrate users due to poor UX Needs RCA a critical or high priority issue that needs an RCA Production Query & JS Pod Issues related to the query & JS Pod Regressed Scenarios that were working before but have now regressed Task A simple Todo Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable
Projects
None yet
Milestone
No milestone
5 participants
@ohansFavour @ramsaptami @arunvjn @nidhi-nair @iamakulov