Adding simple Cypress tests #5693

michellethomas · 2018-08-22T00:59:41Z

I've started to create a few cypress tests and want to get some eyes on it and get the code out so that others can add tests if they'd like. I haven't added it to tox yet but I've run through the steps we'd need to take to run it from the cli.

The general structure for tests that I've included is:
Explore

Controls - setting controls in the UI and confirm that they are getting applied correctly
Links - test all links on explore pages
Visualizations - test functionality for different chart types, use form_data to test various combinations of control values and validate that the data is displaying correctly for the viz type.

Dashboard - testing dashboard loading and editing ability

The difference between control tests and visualization tests seems really minor, but I think it would be useful in thinking about testing in isolation. Removing/setting controls for a single viz type, as well as, validating complex controls like metrics and filters would be useful to separate from testing the parameters for each visualization types. Let me know if anyone has thoughts on this!

I needed to add load_test_users as a cli command so we can run it in backend and frontend tests. I'd want to check that we are in test mode before creating admin users, but it doesn't seem like there's an easy flag to test for test mode. Anyone know of a way to check this? I can add something if not.

Here are the rough steps for running from cli

export SUPERSET_CONFIG=tests.superset_test_config
superset db upgrade
superset load_test_users
superset load_examples
superset init
npm run build
superset runserver &
npm run cypress run

kristw · 2018-08-22T01:02:37Z

superset/assets/package.json

@@ -16,7 +16,8 @@
    "build": "webpack --mode=production --colors --progress",
    "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx .",
    "lint-fix": "eslint --fix --ignore-path=.eslintignore --ext .js,.jsx .",
-    "sync-backend": "babel-node --presets env src/syncBackend.js"
+    "sync-backend": "babel-node --presets env src/syncBackend.js",
+    "cypress": "node_modules/.bin/cypress"


"cypress": "cypress"

npm run will resolve CLI installed under node_modules

michellethomas · 2018-08-22T01:11:35Z

superset/assets/cypress/integration/dashboard/dashboard_tests.js

+    cy.visit('/superset/dashboard/births');
+
+    cy.route('POST', '/superset/explore_json/**').as('getJson');
+    cy.wait(6000, ['@getJson']);


Setting a specific time to wait is usually not necessary, but there's currently an open issue to better handle multiple matched responses. For now this is the workaround: cypress-io/cypress#477

codecov-io · 2018-08-22T06:46:46Z

Codecov Report

Merging #5693 into master will increase coverage by 0.05%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #5693      +/-   ##
==========================================
+ Coverage   63.72%   63.77%   +0.05%     
==========================================
  Files         367      367              
  Lines       23173    23205      +32     
  Branches     2603     2603              
==========================================
+ Hits        14767    14799      +32     
  Misses       8391     8391              
  Partials       15       15

Impacted Files	Coverage Δ
superset/data/__init__.py	`100% <ø> (ø)`	⬆️
superset/assets/src/explore/components/Control.jsx	`38.09% <ø> (ø)`	⬆️
superset/cli.py	`53.81% <96.87%> (+7.21%)`	⬆️
superset/viz.py	`81.42% <0%> (+0.06%)`	⬆️

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 0c98ecb...562e6cf. Read the comment docs.

kristw

Thank you for setting this up. Will be super useful!

kristw · 2018-08-22T17:17:50Z

superset/assets/cypress/integration/dashboard/dashboard_tests.js

+      xhrs.forEach((data) => {
+        expect(data.status).to.eq(200);
+        expect(data.response.body).to.have.property('error', null);
+        cy.get(`#slice-container-${data.response.body.form_data.slice_id}`);


What does this line do?

This would verify that there exists a slice container in the dashboard for each slice loaded.

does it for sure fail in the case that it doesn't exist? is it better/safer to chain a .then() with an expect?

Yes a get will fail with CypressError: Timed out retrying: Expected to find element the two expects above should always catch before the last one does, but seems good to have our bases covered.

kristw · 2018-08-22T17:21:15Z

superset/assets/cypress/integration/dashboard/dashboard_tests.js

+        cy.get(`#slice-container-${data.response.body.form_data.slice_id}`);
+      });
+    });
+    cy.get('#app').then((data) => {


Perhaps this should be nested inside the then block of cy.get('@getJson.all') to ensure that sliceData already has value when it reach this test.

If I understand correctly, both cy.get('@getJson.all') and cy.get('#app') are async so their then blocks are not guaranteed to be in order, unless #app will not exist until @getJson.all resolve.

Because there's a set time to wait on getJson above, the other requests don't start until it's done (which would mean that both cy.get('@getJson.all') and cy.get('#app') don't execute until the 7000 milliseconds is up). In this special case it doesn't specifically matter for their blocks to be in order, but it does seem useful to change because it will be necessary in other cases.

kristw · 2018-08-22T17:23:54Z

superset/assets/cypress/integration/explore/visualization_tests.js

+      optionName: 'metric_1de0s4viy5d_ly7y8k6ghvk',
+    }];
+
+    const formData = Object.assign(FORM_DATA_DEFAULTS, metrics);


Should be

Object.assign({}, FORM_DATA_DEFAULTS, metrics);

To create a copy and put all the fields of FORM_DATA_DEFAULTS and metrics into the new object.
The current code will modify FORM_DATA_DEFAULTS.
You can also use this object spread notation

const formData = { ...FORM_DATA_DEFAULTS, ...metrics }

kristw · 2018-08-22T17:25:28Z

superset/assets/cypress/support/commands.js

+});
+
+Cypress.Commands.add('visitChart', ({ name, sliceId, formData }) => {
+  const baseUrl = '/superset/explore/?form_data=';


take it out of this function so this constant won't be created every time visitChart is called.

michellethomas · 2018-08-23T18:18:14Z

@mistercrunch @graceguo-supercat @john-bodley Initial Cypress integration test PR is ready if you want to take a look!

michellethomas · 2018-08-30T18:54:51Z

@mistercrunch @graceguo-supercat let me know if you have any thoughts about cypress integration tests

kristw

LGTM

williaster

Had a couple of suggested changes, but this looks great overall! Thanks for getting this all started 🚀

williaster · 2018-08-30T21:15:42Z

superset/assets/cypress/integration/dashboard/dashboard_tests.js

+      xhrs.forEach((data) => {
+        expect(data.status).to.eq(200);
+        expect(data.response.body).to.have.property('error', null);
+        cy.get(`#slice-container-${data.response.body.form_data.slice_id}`);


does it for sure fail in the case that it doesn't exist? is it better/safer to chain a .then() with an expect?

williaster · 2018-08-30T21:19:14Z

superset/assets/cypress/integration/explore/visualization_tests.js

+      optionName: 'metric_1de0s4viy5d_ly7y8k6ghvk',
+    }];
+
+    const formData = Object.assign({}, FORM_DATA_DEFAULTS, { metrics });


can we use object rest spread? or is this code not transpiled?

Is there a reason object rest spread is better in this situation?

because it's modern javascript and part of the airbnb best practices. I think we should strive to follow them, and we should have this lint rule enabled ... but we don't because we have a lot turned off which arguably should be on.

williaster · 2018-08-30T21:21:46Z

superset/assets/cypress/support/commands.js

+// -- This is will overwrite an existing command --
+// Cypress.Commands.overwrite("visit", (originalFn, url, options) => { ... })
+
+const BASE_URL = '/superset/explore/?form_data=';


is BASE_EXPLORE_URL a better name since this is not used as the base for every request?

williaster · 2018-08-30T21:23:22Z

superset/assets/cypress/support/commands.js

+  });
+});
+
+Cypress.Commands.add('visitChart', ({ name, sliceId, formData }) => {


when I first read this it wasn't clear to me what visitChart meant. While it seems possibly useful to have a method that could handle 3 different types of input, since they are all mutually exclusive could we instead create 3 methods with more descriptive names which each expect one type of input?

williaster · 2018-08-30T21:24:11Z

superset/assets/src/explore/components/Control.jsx

@@ -84,6 +84,7 @@ export default class Control extends React.PureComponent {
    const divStyle = this.props.hidden ? { display: 'none' } : null;
    return (
      <div
+        data-test={this.props.name}


should this be removed? I think it's not ideal to have props set solely for tests.

this was listed as a best practice by cypress to avoid using css selectors that may change https://docs.cypress.io/guides/references/best-practices.html#Selecting-Elements

but we use css selectors elsewhere in these tests right? that seems unavoidable to some extent. If you feel strongly about it let's go with this pattern but perhaps you could set it to data-name so it indicates what the value is rather than just 'test'?

It seemed ok to use some id / class selectors that are less likely to change. But when I google around for blogs discussing making UI tests resilient to change, this pattern comes up often.

This was a particular case where there was no identifier on the tag that was unique to it being a metric control (vs other type of control), so I needed to add some kind of id or identifier here, seemed good to be clear that it's just used for testing (so we know not to remove / change it). That's the reason I like using data_test, but I can change it if you feel strongly.

got it, thanks for more context. I'm cool with whatever you feel is best here 👍

michellethomas · 2018-09-06T17:35:28Z

I added Cypress to tox. It takes 12 minutes but most of that time is in setup. Let me know if anyone has other feedback.

john-bodley · 2018-09-06T19:23:00Z

tox.ini

@@ -74,4 +89,5 @@ envlist =
    flake8
    javascript
    pylint
+    cypress


Nit. Alphabetical order.

john-bodley · 2018-09-06T19:23:18Z

tox.ini

+    -rrequirements.txt
+    -rrequirements-dev.txt
+
+


Nit. Extra blank line.

john-bodley · 2018-09-06T19:23:50Z

tox.ini

@@ -61,6 +63,19 @@ commands =
    {toxinidir}/superset/assets/js_build.sh
 deps =

+[testenv:cypress]


Nit. Alphabetical order of test environments.

john-bodley · 2018-09-06T19:29:24Z

tox.ini

+commands =
+    {toxinidir}/superset/assets/cypress_build.sh
+setenv =
+    SUPERSET_CONFIG = tests.superset_test_config


Nit. Alphabetical order.

* Adding simple Cypress tests * Changing visitChart into multiple commands * Adding Cypress to tox

kristw reviewed Aug 22, 2018

View reviewed changes

michellethomas commented Aug 22, 2018

View reviewed changes

michellethomas force-pushed the adding_cypress_tests branch 3 times, most recently from 526ade8 to 40da12c Compare August 22, 2018 01:47

michellethomas changed the title ~~Adding simple Cypress tests~~ [WIP] Adding simple Cypress tests Aug 22, 2018

michellethomas force-pushed the adding_cypress_tests branch from 5b3636e to a325999 Compare August 22, 2018 06:23

michellethomas changed the title ~~[WIP] Adding simple Cypress tests~~ Adding simple Cypress tests Aug 22, 2018

kristw reviewed Aug 22, 2018

View reviewed changes

michellethomas force-pushed the adding_cypress_tests branch 2 times, most recently from c34a28e to 202e38b Compare August 22, 2018 18:10

kristw approved these changes Aug 30, 2018

View reviewed changes

williaster reviewed Aug 30, 2018

View reviewed changes

michellethomas force-pushed the adding_cypress_tests branch 11 times, most recently from 1ef2108 to e5906f5 Compare September 6, 2018 05:10

john-bodley reviewed Sep 6, 2018

View reviewed changes

tox.ini Outdated

@@ -74,4 +89,5 @@ envlist =

flake8

javascript

pylint

cypress

Copy link

Member

john-bodley Sep 6, 2018

Choose a reason for hiding this comment

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

Nit. Alphabetical order.

john-bodley reviewed Sep 6, 2018

View reviewed changes

tox.ini Outdated

-rrequirements.txt

-rrequirements-dev.txt

Copy link

Member

john-bodley Sep 6, 2018

Choose a reason for hiding this comment

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

Nit. Extra blank line.

john-bodley reviewed Sep 6, 2018

View reviewed changes

michellethomas added 2 commits September 6, 2018 13:39

Adding simple Cypress tests

410089f

Changing visitChart into multiple commands

3303fa0

michellethomas force-pushed the adding_cypress_tests branch 2 times, most recently from d463f53 to f423f9c Compare September 6, 2018 21:15

Adding Cypress to tox

562e6cf

michellethomas force-pushed the adding_cypress_tests branch from f423f9c to 562e6cf Compare September 6, 2018 21:16

john-bodley merged commit d40ded0 into apache:master Sep 6, 2018

michellethomas mentioned this pull request Sep 6, 2018

Add support for integration tests #5405

Closed

arpit-agarwal mentioned this pull request Oct 5, 2018

feat: import/export dashboards via cli #5991

Merged

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018

Adding simple Cypress tests (apache#5693)

321f811

* Adding simple Cypress tests * Changing visitChart into multiple commands * Adding Cypress to tox

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Adding simple Cypress tests (apache#5693)

d0eeca1

* Adding simple Cypress tests * Changing visitChart into multiple commands * Adding Cypress to tox

mistercrunch mentioned this pull request Mar 6, 2019

Remove unused "get_or_create_main_db" statements #6984

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding simple Cypress tests #5693

Adding simple Cypress tests #5693

michellethomas commented Aug 22, 2018 •

edited

Loading

kristw Aug 22, 2018

michellethomas Aug 22, 2018

codecov-io commented Aug 22, 2018 •

edited

Loading

kristw left a comment

kristw Aug 22, 2018

michellethomas Aug 22, 2018

williaster Aug 30, 2018

michellethomas Aug 31, 2018

kristw Aug 22, 2018

michellethomas Aug 22, 2018

kristw Aug 22, 2018

kristw Aug 22, 2018

michellethomas commented Aug 23, 2018

michellethomas commented Aug 30, 2018

kristw left a comment

williaster left a comment •

edited

Loading

williaster Aug 30, 2018

williaster Aug 30, 2018

michellethomas Aug 31, 2018

williaster Aug 31, 2018 •

edited

Loading

williaster Aug 30, 2018

williaster Aug 30, 2018

williaster Aug 30, 2018 •

edited

Loading

michellethomas Aug 31, 2018

williaster Aug 31, 2018

michellethomas Aug 31, 2018

williaster Aug 31, 2018

michellethomas commented Sep 6, 2018

john-bodley Sep 6, 2018

john-bodley Sep 6, 2018

john-bodley Sep 6, 2018

john-bodley Sep 6, 2018

Adding simple Cypress tests #5693

Adding simple Cypress tests #5693

Conversation

michellethomas commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 22, 2018 • edited Loading

Codecov Report

kristw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michellethomas commented Aug 23, 2018

michellethomas commented Aug 30, 2018

kristw left a comment

Choose a reason for hiding this comment

williaster left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michellethomas commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michellethomas commented Aug 22, 2018 •

edited

Loading

codecov-io commented Aug 22, 2018 •

edited

Loading

williaster left a comment •

edited

Loading

williaster Aug 31, 2018 •

edited

Loading

williaster Aug 30, 2018 •

edited

Loading