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

🐛 fix #1098 : also persist 0, empty string etc #1142

Merged
merged 1 commit into from
Mar 22, 2020

Conversation

eddy-geek
Copy link
Contributor

@eddy-geek eddy-geek commented Mar 3, 2020

fixes #1098 Persistence does not work for Input components when 0 is entered

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

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 number updates the dash docs
    • here is the show and tell thread in plotly dash community

@alexcjohnson
Copy link
Collaborator

Thanks @eddy-geek! The fix looks good, the only additional pieces we need are:

  • a test locking down the correct behavior - could just be an addition to rdps011 storing an empty string, or a new test of perhaps persistence of a dropdown with various falsy values.
  • a CHANGELOG entry.

@eddy-geek
Copy link
Contributor Author

💔 Note on testing dash

I have run the tests locally and they passed. (refer to testing section in contributing)

the testing section does not have any commands. I tried this .circleci snippet:

pytest --headless --nopercyfinalize --junitxml=test-reports/junit_intg.xml

but it fails with ModuleNotFoundError: No module named 'dash_renderer_test_components' apparently imported from renderer/test_multi_output.py.
so back to circleci, I adapted (needs review!) to:

git clone --depth 1 https://github.com/plotly/dash-renderer-test-components
cd dash-renderer-test-components && npm ci && npm run build:all && python setup.py sdist && pip install -e . && cd ..

next! chromedriver missing, I had to wget https://chromedriver.storage.googleapis.com/80.0.3987.106/chromedriver_linux64.zip ; unzip chromedriver_linux64.zip ;mv chromedriver ~/.local/bin

And finally it worked (with some usual chromedriver instability)

Is that the proper way or did I miss something? Shouldn't this be in the contributing guide?

@eddy-geek eddy-geek force-pushed the eddy-geek-persistence branch 2 times, most recently from 4e671aa to fc4dad2 Compare March 22, 2020 12:14
@eddy-geek
Copy link
Contributor Author

@alexcjohnson ready for review. I have checked that the changed test now fails as expected without the fix, with:

$ git checkout HEAD^ dash-renderer/src/persistence.js
$ cd dash-renderer ; npm run build ; pip install -e . ; cd ..
$ pytest --headless  tests/integration/renderer/test_persistence.py::test_rdps010_toggle_persistence
>       dash_duo.wait_for_text_to_equal('#out', '')
E       selenium.common.exceptions.TimeoutException: Message: text ->  not found within 10s

fixes plotly#1098 Persistence does not work for Input components when 0 is entered
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.

Excellent, thanks! 💃

Good point about the test setup and commands - we're intending to simply remove dash-renderer-test-components (building the relevant functionality into the tests directly) but we should add a note about chromedriver. And for running the tests locally, it should suffice to just run pytest plain.

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.

[BUG] Persistence does not work for Input components when 0 is entered
2 participants