-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Pytest fixtures #744
Conversation
@alexcjohnson this is still WIP, but would like to get some early feedbacks. |
from dash.exceptions import PreventUpdate | ||
|
||
|
||
def test_dveh001_python_errors(dash_duo): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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'] |
There was a problem hiding this comment.
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.
…ed before release
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 .. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
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
optionals
CHANGELOG.md