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

fix: Cypress tests reliability improvements #19800

Merged

Conversation

diegomedina248
Copy link
Contributor

@diegomedina248 diegomedina248 commented Apr 21, 2022

SUMMARY

This PR aims to solve the intermittent issues we're having with our integration tests, to make them deterministic.

Exploration & findings

The issue we're having, seemingly randomly, is with one of the following:

  • AdhocFilters
  • AdhocMetrics
  • Advanced analytics
  • Annotations

Some examples of these tests failing in the past:

Looking at several of these errors, the snapshots show something very similar to each other:
AdhocMetrics -- Clear metric and set simple adhoc metric -- before each hook (failed) (attempt 3)

The error The URL is missing the dataset_id or slice_id parameters. occurs when these params are not found.
Back to the tests, we're trying to explore the same chart, Num Births Trend, by performing the following action:

cy.visitChartByName('Num Births Trend');

In turn, visitChartByName, does the following:

cy.request(`/chart/api/read?_flt_3_slice_name=${name}`).then(response => {
  cy.visit(`${BASE_EXPLORE_URL}{"slice_id": ${response.body.pks[0]}}`);
});

So, we try to fetch the slice information by name, to get the id and route to the explore.
The /chart/api/read works (aka, returns 200) for the failed tests, but, it returns 0 results.
So, that's why we're seeing the error, and why the test is failing.

Root cause

The chart we're trying to access exists, and the test is correct.
If it's not deterministic then, it must be that other test is messing with the data these tests are using.
I can think of two potential causes:

  1. The chart is renamed by other test
  2. The chart is deleted by other test

Investigating, I found a test inside chart list view suite that performs a bulk delete, which seems to be the culprit.
This test bulk deletes the first two charts.

This explains the issue and why the tests are non-deterministic:

  • We have a test that potentially deletes our chart
  • That deletion could delete different charts depending on execution

This happens because, when we load the chart list view, the charts are ordered by modification date, which means, if other test modified the Num Births Trend chart before the bulk delete, then that chart is getting deleted.

Solution

If we sort the chart list view by name, then we ensure this test always deletes the same charts, and it does not impact the charts other tests rely on.

Note

Our integration tests have dependencies (whether expected or not) and we need to consider/restructure the execution to ensure we get the same deterministic result, always.

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

@diegomedina248 diegomedina248 changed the title fix: Cypress tests reliability improvements [WIP] fix: Cypress tests reliability improvements Apr 21, 2022
@diegomedina248 diegomedina248 marked this pull request as ready for review April 21, 2022 00:34
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #19800 (9236c2f) into master (22a92ed) will increase coverage by 0.14%.
The diff coverage is 80.86%.

❗ Current head 9236c2f differs from pull request most recent head bb12964. Consider uploading reports for the commit bb12964 to get more accurate results

@@            Coverage Diff             @@
##           master   #19800      +/-   ##
==========================================
+ Coverage   66.40%   66.55%   +0.14%     
==========================================
  Files        1690     1692       +2     
  Lines       64732    64801      +69     
  Branches     6656     6657       +1     
==========================================
+ Hits        42987    43128     +141     
+ Misses      20046    19973      -73     
- Partials     1699     1700       +1     
Flag Coverage Δ
javascript 51.25% <70.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 33.33% <ø> (ø)
...rset-ui-chart-controls/src/sections/chartTitle.tsx 100.00% <ø> (ø)
...omponents/ColumnConfigControl/ColumnConfigItem.tsx 0.00% <ø> (ø)
.../shared-controls/components/RadioButtonControl.tsx 0.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 33.33% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 100.00% <ø> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
.../legacy-plugin-chart-world-map/src/controlPanel.ts 100.00% <ø> (ø)
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <0.00%> (ø)
... and 56 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 22a92ed...bb12964. Read the comment docs.

@diegomedina248 diegomedina248 force-pushed the fix/cypress-tests-improvements branch 6 times, most recently from 53b094c to 3157c1e Compare April 21, 2022 14:34
@diegomedina248 diegomedina248 force-pushed the fix/cypress-tests-improvements branch 14 times, most recently from d7c2583 to 1ba4e09 Compare April 22, 2022 15:53
@diegomedina248 diegomedina248 changed the title [WIP] fix: Cypress tests reliability improvements fix: Cypress tests reliability improvements Apr 22, 2022
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

THANK YOU!

@rusackas rusackas merged commit 3f0413b into apache:master Apr 22, 2022
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@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-Patch size/S 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants