Uh oh!
There was an error while loading. Please reload this page.
Remove events - #114
Conversation
There were some (mostly skipped) tests of events, and rather than removing them, I adapted them to the corresponding event properties. This had the nice effects of 1) showing that the event behavior that was listed as broken is not broken as properties, and 2) pointing out the need to skip the first call (using PreventUpdate) if we want an exact match to the old event behavior.
| "format:test": "prettier --config .prettierrc src/**/*.js --list-different", | ||
| "test": "npm run lint" | ||
| "test": "npm run lint", | ||
| "test:py": "python -m unittest tests.test_render.Tests && python -m unittest tests.test_race_conditions.Tests" |
There was a problem hiding this comment.
Please update CircleCI config to use this command instead
Marc-Andre-Rivet
commented
Jan 10, 2019
I think this |
Marc-Andre-Rivet
commented
Jan 10, 2019
Missing a changelog entry. |
Getting errors when running dash-docs with this version of dash-renderer. Need to remove the dash-renderer/src/actions/index.js Line 704 in c21512c Not sure if this is what's getting caught by the failing tests or something else. |
alexcjohnson
commented
Jan 18, 2019
Good catch! updated that in c4a9a22. It seems like the dash-renderer tests don't touch this unfortunately, but it's a bit tough as I get a few failures with these tests locally even on master. Not going to try and sort that out in this PR, but I'll see if I can add a test that covers that |
| if not n_clicks: | ||
| # initial value is quick, only new value is slow | ||
| # also don't let the initial value increment call_counts | ||
| return 'Initial Value' |
There was a problem hiding this comment.
I don't understand how this test was passing on master - it failed for me locally, and failed in this branch on ci, until this fix.
| if get_message: | ||
| message = get_message() | ||
| elif expected_value: | ||
| message = 'Final value: {}'.format(condition_val) |
There was a problem hiding this comment.
Would have helped me debug test_removing_component_while_its_getting_updated - I thought the callback wasn't getting called at all, when in fact it was called too many times! With expected_value you can get error messages like:
======================================================================
ERROR: test_removing_component_while_its_getting_updated (tests.test_render.Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/test_render.py", line 1423, in test_removing_component_while_its_getting_updated
wait_for(lambda: call_counts['button-output'].value, expected_value=1)
File "tests/utils.py", line 72, in wait_for
raise WaitForTimeout(message)
WaitForTimeout: Final value: 2
Which would have made it much clearer what was going on.
Would be nice if we could share utils like this across dash repos - perhaps if we split out dash_development things like this could go there?
alexcjohnson
commented
Jan 20, 2019
Aaaactually we did have a test that covered this case: |
Marc-Andre-Rivet
commented
Jan 20, 2019
@alexcjohnson So, I haven't digged enough in renderer to have a fully formed opinion but a few things already come to mind -- obviously nothing in here is in the scope for this PR!
|
Closes#113
Pretty straightforward, just removing the
DepGraphfor events (but note thatstate.graphsis still an object we can add more to, which may be important for multi-outputs and dynamic callbacks to create a modified graph for cycle detection), and all references tofireEvent.As in plotly/dash-html-components#89 I added a test shortcut to package.json - is there already a way to do this that I didn't see?