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.

Sign up for GitHub

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

Jump to bottom

Follow up on echo updates #3407

Merged
merged 6 commits into from
Mar 8, 2022
Merged

Follow up on echo updates #3407

merged 6 commits into from
Mar 8, 2022

Conversation

jasongrout
Copy link
Member

@jasongrout jasongrout commented Mar 4, 2022 •
edited
Loading

This follows up on #3394 in adding tests, etc.

Fixes #3404

  • We should add one or more tests for the behavior when we set the env var to use / not use echos. Currently this is not covered by the tests? (This is done in Backport echo_update messages to 7.x #3400 for 7.x)
  • We should add tests for the JS side of echos. Currently there are no tests checking this, and I think I could simplify the JS code for echo handling some more, but it is hard to be certain since non of the expected behavior is encoded in tests.
  • Can we add a note in the docstring of send_sync_message about what the expected return value is? E.g. "If a message is sent successfully, this returns the message ID of that message. Otherwise it returns an empty string".
  • Change the private variables to start with _.

@jasongrout
Add tests for setting JUPYTER_WIDGETS_ECHO to False
6cccc9c 
This copies the test_set_state file from before jupyter-widgets#3195
to make sure that it still works unchanged when disabling
echo_update messages.
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

Binder 👈 Launch a binder notebook on branch jasongrout/ipywidgets/testnoecho

@jasongrout jasongrout changed the title Add tests for setting JUPYTER_WIDGETS_ECHO to False Follow up on echo updates Mar 4, 2022
@jasongrout jasongrout added this to the 8.0 milestone Mar 4, 2022
@jasongrout
Copy link
Member Author

@vidartf - can you review these follow-up items that you requested?

@jasongrout jasongrout mentioned this pull request Mar 4, 2022
Closed
3 tasks
@vidartf
Proposal to better keep tests in sync
94ffe26 
As the code in test_set_state_noecho was mostly a copy, I tried to make it more DRY.
vidartf
vidartf reviewed Mar 7, 2022
View reviewed changes
Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

Thanks! I added some smaller comments, and pushing a commit to make the echo a parameter for the tests to make it more DRY.

packages/base/test/src/widget_test.ts Show resolved Hide resolved
packages/base/test/src/widget_test.ts Show resolved Hide resolved
@jasongrout
Copy link
Member Author

I added some smaller comments, and pushing a commit to make the echo a parameter for the tests to make it more DRY.

Thanks!

@vidartf vidartf merged commit ac7c4b9 into jupyter-widgets:master Mar 8, 2022
@jtpio jtpio mentioned this pull request Aug 4, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vidartf vidartf vidartf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

Follow-up items for echoing state
2 participants
@jasongrout @vidartf