Skip to content

Rewrite tests with dash testing #153

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

Merged
merged 28 commits into from
Oct 6, 2021
Merged

Conversation

kinimesi
Copy link
Contributor

@kinimesi kinimesi commented Aug 23, 2021

About

Fixes #118, rewrite the tests with dash[testing]

Description of changes

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Cytoscape core contributor.

Reference Issues

Closes #[issue number]

Other comments

@xhluca
Copy link

xhluca commented Aug 30, 2021

Hey on a first look the code seems fine, however there are a lot of missing images:
image

Can you investigate why those are not being generated? should be about 100 images if you run the tests locally

@kinimesi
Copy link
Contributor Author

I'll look into it, thanks.

@kinimesi
Copy link
Contributor Author

kinimesi commented Sep 9, 2021

It should be fixed now. However, it needs manual approval. Since dash_duo.percy_snapshot combines the snapshot name with the actual python versions, percy cannot match them by name to visually compare (that's why right now it says 103 viaul changes need review)


self.dash_duo.driver.save_screenshot(path)

def test_callbacks(self, dash_duo):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our usual usage of dash.testing does not create classes at all - just functions, and they should include test case ids, for which our convention is 4 characters to identify the file and three numbers for the test case within the file. So this one might be test_cycb001_callbacks(dash_duo), in test_interactions.py they might be test_cyin001_dragging, test_cyin002_clicking etc... the advantage here is you can easily run exactly the test you want using for example pytest -k cyin002.

Helper functions like create_input_and_save can either be nested inside the test case (if they're only used in one place like in this file) or they can be explicitly passed dash_duo

@@ -74,36 +68,20 @@ def display_image(pathname): # pylint: disable=W0612

return app

def percy_snapshot(self, name=''):
@staticmethod
def percy_snapshot(dash_duo, name=''):
if os.environ.get('PERCY_ENABLED', False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to explicitly check PERCY_ENABLED - dash_duo.percy_snapshot is a noop if percy is disabled.

)
time.sleep(2)

def test_usage(self):
# Create and start the app
def test_usage(self, dash_duo):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole set of tests would be a great use case for pytest.mark.parametrize, eg:

import pytest

@pytest.mark.parametrize('name', ['usage', 'elements', 'layouts', 'style'])
def test_cyps001_snapshots(name, dash_duo):
    app = self.create_app(dir_name=name)
    dash_duo.start_server(app)

    run_percy_on(name, dash_duo)

def test_usage_advanced(self):
self.create_usage_test('usage-advanced')
def test_usage_advanced(self, dash_duo):
self.create_usage_test(dash_duo, 'usage-advanced')
Copy link
Collaborator

Choose a reason for hiding this comment

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

pytest.mark.parametrize again :)

@kinimesi
Copy link
Contributor Author

kinimesi commented Sep 9, 2021

Thanks @alexcjohnson I mainly rewrote the existing code by using dash.testing and kept the existing structure. I'll update them based on your feedback.

@jackparmer jackparmer merged commit 29543d0 into master Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite the tests
4 participants