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

add extra wait for callbacks in queue #974

Merged
merged 13 commits into from
Oct 29, 2019
Merged

add extra wait for callbacks in queue #974

merged 13 commits into from
Oct 29, 2019

Conversation

byronz
Copy link
Contributor

@byronz byronz commented Oct 23, 2019

after the discussion with @alexcjohnson about the incident of dash-daq PR on dash docs, this PR adds one extra wait in visit_and_snapshot API to make sure all the callbacks in the queue get fired. tested with the local snapshots and also dash-docs tests, the results are positive.

https://percy.io/plotly/dash-docs/builds/2904660
https://percy.io/plotly/dash/builds/2904645

@byronz byronz marked this pull request as ready for review October 23, 2019 19:14
@alexcjohnson
Copy link
Collaborator

FYI the PR that sparked this push for more robust percy snapshots is https://github.com/plotly/dash-docs/pull/676

This strategy looks great - I didn't know about self.redux_state_rqs, that's really nice to have!

Just the one comment above, then I think this will be good to go.

BTW I had to approve one spurious percy diff here that seems to be frequently changing, chapter-1-again (I've also seen chapter-1 fail similarly). I think it's somehow autosizing the graph differently in different runs? If that's it, we can 🔒 it by changing this line:

to have fixed height & width like:

"layout": {"title": value, "width": 500, "height": 400}

@byronz
Copy link
Contributor Author

byronz commented Oct 23, 2019

This strategy looks great - I didn't know about self.redux_state_rqs, that's really nice to have!

it's in the dash_page.py which is not documented, we should add a section for that part later

@alexcjohnson
Copy link
Collaborator

Oh actually, one more concern: what if some existing cases are snapshotting pages that don't even have callbacks? I guess we could look at store.getState().dependenciesRequest.content.length - if that's 0, don't wait for anything to show up in requestQueue?

@byronz
Copy link
Contributor Author

byronz commented Oct 24, 2019

Oh actually, one more concern: what if some existing cases are snapshotting pages that don't even have callbacks? I guess we could look at store.getState().dependenciesRequest.content.length - if that's 0, don't wait for anything to show up in requestQueue?

In the context of dash docs (where we use mostly this API) there is always at least one request like the home page.

In the context of an app, you are right that no callback, no requestqueue, but I'm also wondering the purpose of using this API in this case. isn't a simple percy_snapshot more straightforward?

@alexcjohnson
Copy link
Collaborator

isn't a simple percy_snapshot more straightforward?

Oh... right, I forgot about the visit_and_snapshot vs percy_snapshot distinction - in fact if it were robust (against not even being a dash page, for example what if you get a wekzeug debug page) I could see this being part of percy_snapshot - perhaps with the ability to turn it off in case you actually want to run ahead of a response for some reason.

@byronz
Copy link
Contributor Author

byronz commented Oct 24, 2019

isn't a simple percy_snapshot more straightforward?

Oh... right, I forgot about the visit_and_snapshot vs percy_snapshot distinction - in fact if it were robust (against not even being a dash page, for example what if you get a wekzeug debug page) I could see this being part of percy_snapshot - perhaps with the ability to turn it off in case you actually want to run ahead of a response for some reason.

we don't have window.store outside the scope of dash apps. :)

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.

Love it, wait_for_callbacks is easy, clear, and explicit.
Add a CHANGELOG entry, then 💃

@byronz byronz merged commit d4fed80 into dev Oct 29, 2019
@byronz byronz deleted the test-visit-and-snapshot branch October 29, 2019 11:57
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.

None yet

2 participants