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

Pytest fixtures #744

Merged
merged 54 commits into from
Jun 10, 2019
Merged

Pytest fixtures #744

merged 54 commits into from
Jun 10, 2019

Conversation

byronz
Copy link
Contributor

@byronz byronz commented May 23, 2019

Start with a description of this PR. Then edit the list below to the items that make sense for your PR scope, and check off the boxes as you go!

Contributor Checklist

this fix #728

  • I have broken down my PR scope into the following TODO tasks
    • fixture for app runner (flask server) (thread and process)
    • fixture for selenium webdriver
    • combo fixture binding thread + selenium for typical integration case
    • migrate a set of existing test cases using the fixtures
  • I have run the tests locally and they passed. (refer to testing section in contributing)

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
    • this github #PR 520 updates the dash docs
    • update the pip install description about the pytest fixtures
    • here is the show and tell thread in plotly dash community
    • update the testing part in contributing guide Update CONTRIBUTING.md #772

@byronz byronz marked this pull request as ready for review May 31, 2019 18:56
@byronz
Copy link
Contributor Author

byronz commented May 31, 2019

@alexcjohnson this is still WIP, but would like to get some early feedbacks.

.pylintrc Outdated Show resolved Hide resolved
dash/exceptions.py Outdated Show resolved Hide resolved
from dash.exceptions import PreventUpdate


def test_dveh001_python_errors(dash_duo):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson the tcid is added, it's more precise to communicate the test case with a unique id. and in case of test selection, mostly of time you can play with pytest -k tcid.

the current convention is mmffddd abbr for module name and file and three digits, it also helps to navigate to the test case in IDE if you have the tcid

"title": "tooltip",
"style": {"color": "red", "fontSize": 30},
}
# fmt:off
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in general, I'm happy with the unarguable black default style, and here is an exception I explicitly turned it off


rqs = dash_duo.redux_state_rqs
assert len(rqs) == 1
assert rqs[0]["controllerId"] == "output-1.children" and not rqs[0]['rejected']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the abstraction we did before assertion_request_queue, it encapsulates the checkpoints elsewhere, and someone also introduces a bug by inserting another parameter, but forget to check all the API calls, so we end up having several API calls to check the length of the queue, but it actually passed to the first added parameter which is a boolean.

perhaps the only acceptable exception to hide the assertion with an API is the check below about clear browser logs.

@byronz byronz added the dash 1.0 label Jun 7, 2019
git clone --depth 1 https://github.com/plotly/dash-table.git
git clone --depth 1 https://github.com/plotly/dash-renderer-test-components
. venv/bin/activate
cd dash-core-components && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
cd dash-html-components && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
cd dash-core-components && git checkout 2932409 && npm install --ignore-scripts && npm run build && pip install -e . && cd ..
Copy link
Contributor Author

Choose a reason for hiding this comment

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

after these two commits, dev bundle was removed in dcc and html, which was the root cause of the 4 devtools props-check related failure.

we need to address that with the formal release and this is a workaround for this PR to pass all tests

poll=0.1,
msg="expected condition not met within timeout",
): # noqa: C0330
res = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, but do we want to init with a wait_cond() so if it's already satisfied we don't even need to wait one poll period?

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! I made a couple of small comments, the only real important one is if we should change start_app_server to start_server, since that would be a breaking change if done later. Aside from that, it's a 💃 from me, but let's let @Marc-Andre-Rivet comment about whether to sort out the build issue here or in a separate PR.

@byronz byronz changed the title WIP - Pytest fixtures Pytest fixtures Jun 10, 2019
@byronz byronz merged commit 84133a3 into master Jun 10, 2019
@alexcjohnson alexcjohnson deleted the pytest-fixtures branch June 10, 2019 16:55
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.

merge pytest fixtures
2 participants