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.

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(native filter): refactor and add new test #19821

Merged
merged 13 commits into from
Apr 28, 2022

Conversation

jinghua-qa
Copy link
Member

@jinghua-qa jinghua-qa commented Apr 22, 2022

SUMMARY

1, Put native filter related function to nativeFilter.helpers.ts
2, Separated native filter tests depend on different initial state
3, add new native filter tests:
User can stop filtering when filter is removed
User can create a numerical range filter
User can remove parent filters
User can apply value filter with selected values
Verify that default value is respected after revisit

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Apr 23, 2022

Codecov Report

Merging #19821 (a1bbfb2) into master (fa68036) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #19821      +/-   ##
==========================================
- Coverage   66.54%   66.50%   -0.05%     
==========================================
  Files        1692     1714      +22     
  Lines       64807    64999     +192     
  Branches     6661     6713      +52     
==========================================
+ Hits        43129    43226      +97     
- Misses      19978    20065      +87     
- Partials     1700     1708       +8     
Flag Coverage Δ
javascript 51.24% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <0.00%> (-19.05%) ⬇️
...end/src/components/Form/LabeledErrorBoundInput.tsx 95.00% <0.00%> (-5.00%) ⬇️
...-ui-chart-controls/src/components/MetricOption.tsx 90.90% <0.00%> (-3.83%) ⬇️
...frontend/src/SqlLab/components/SqlEditor/index.jsx 51.07% <0.00%> (-0.28%) ⬇️
superset-frontend/src/views/CRUD/hooks.ts 46.15% <0.00%> (-0.26%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 60.39% <0.00%> (ø)
...-frontend/src/components/AlteredSliceTag/index.jsx 87.87% <0.00%> (ø)
...-frontend/src/visualizations/presets/MainPreset.js 100.00% <0.00%> (ø)
...ntend/src/views/CRUD/annotation/AnnotationList.tsx 69.35% <0.00%> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <0.00%> (ø)
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa68036...a1bbfb2. Read the comment docs.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jinghua-qa Thanks for improving the tests! I left some comments 😉

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jinghua-qa I executed the tests locally and it's really slow. The main reason is that we are recreating the dashboard and waiting for all charts to be loaded for each test as you can see in the beforeEach below:

beforeEach(() => {
  cy.login();
  cleanUp();
  copyTestDashboard("World Bank's Data");
  WORLD_HEALTH_CHARTS.forEach(waitForChartLoad);
  closeDashboardToastMessage();
});

The majority of tests don't need a fresh initial state to execute. We can start the initial state with an existing filter and handle these types of tests without recreating the whole dashboard:

  • 'User can expand / retract native filter sidebar on a dashboard
  • Verify setting options and tooltips for value filter
  • User can check 'Filter has default value
    ...

For tests like:

  • User can create a time range filter
  • User can create a time grain filter
  • User can create a time column filter
    ...

We can just add new filters to execute the tests.

Only a few tests will required a complete fresh initial state. Making these suggested changes will greatly improve execution time.

@michael-s-molina
Copy link
Member

Can you also add a PR description with the summary of your changes?

@zhaoyongjie zhaoyongjie self-requested a review April 28, 2022 08:37
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled the branch and tested it in my local. It looks like the biggest extra overhead in the current Cypress Test is repeatedly login() and refresh tested page in every test case. There is a new feature introduced from Cypress Community.
IMO, we need to upgrade to the latest Cypress and try this feature. Of course, this work is not related to this PR. The PR intends to upgrade the cypress.

others, LGTM.

@jinghua-qa
Copy link
Member Author

Can you also add a PR description with the summary of your changes?

Done, added PR description

@jinghua-qa
Copy link
Member Author

@jinghua-qa I executed the tests locally and it's really slow. The main reason is that we are recreating the dashboard and waiting for all charts to be loaded for each test as you can see in the beforeEach below:

beforeEach(() => {
  cy.login();
  cleanUp();
  copyTestDashboard("World Bank's Data");
  WORLD_HEALTH_CHARTS.forEach(waitForChartLoad);
  closeDashboardToastMessage();
});

The majority of tests don't need a fresh initial state to execute. We can start the initial state with an existing filter and handle these types of tests without recreating the whole dashboard:

  • 'User can expand / retract native filter sidebar on a dashboard
  • Verify setting options and tooltips for value filter
  • User can check 'Filter has default value
    ...

For tests like:

  • User can create a time range filter
  • User can create a time grain filter
  • User can create a time column filter
    ...

We can just add new filters to execute the tests.

Only a few tests will required a complete fresh initial state. Making these suggested changes will greatly improve execution time.

Separated the tests into 2 different setup suite

@jinghua-qa jinghua-qa merged commit ad1338f into apache:master Apr 28, 2022
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
* refactor and add new test

* fix lint

* fix fail test

* fix front end error

* fix frontend error

* fix fail test for front end check

* add native filter helper

* more changes

* seperated test for different state

* seperated tests by initail state

* fix failure

* one more fix

* enable test
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* refactor and add new test

* fix lint

* fix fail test

* fix front end error

* fix frontend error

* fix fail test for front end check

* add native filter helper

* more changes

* seperated test for different state

* seperated tests by initail state

* fix failure

* one more fix

* enable test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io Preset-QA size/XXL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants