Update Num_Input to correct for Type Errors #3074
Conversation
|
I do not have access to the docs to be able to correct that failure :) |
|
@RedFrez try merging in the latest from develop. We saw this issue happen and found a fix. Should fix the docs. |
ghost
self-assigned this
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Aside from a small nit, looks good to me!
There was a problem hiding this comment.
Hi @RedFrez! I'm jumping in as reviewer number 2 for this PR as per our standard process
As mentioned in the PR description, this could probably use a bit more test coverage (it should be sufficient to add a single test failing in the old world but passing in the new one)
The best place to add it would be in lib/tests/streamlit/number_input_test.py since I think it's both easiest to add them there and totally sufficient to validate these changes at that level vs in the frontend/e2e
Once a test or two is added this LGTM
vdonato
left a comment
vdonato
left a comment
There was a problem hiding this comment.
LGTM
Will let @bh-streamlit hit the button
|
@vdonato for the cypress failure showing widget correctly.... is this due to using a mac to update the shots? I am not sure how to use Cypress to update them. |
|
@RedFrez: Yeah that's spot on. Unfortunately, our snapshots are dependent on the environment that they're taken in, so taking them in an environment that isn't identical to the image that runs in CI will result in slight differences the differ then picks up. If you have access to the CircleCI-generated artifacts, the way that I generally fix this is to delete all of the newly added ones, let CI run to generate new snapshots, then take those files and commit them (the tests will pass if there are no committed snapshots but will also generate them as artifacts, which is cypress' behavior that I don't quite understand). If you can't get to the CircleCI artifacts, then I'm happy to help out with this. There's an infrastructure ticket (#2682) to revive a now-stale docker image so that we have an environment identical to CI for taking these snapshots that I'll hopefully be able to get around to in the near future. |
|
@vdonato it would be helpful if you could generate the snapshots. The docker container seems to install without any problem, but then I run into access errors while trying to run the frontend. |
|
@RedFrez - sure! I just made a PR on your fork of the repo to use snapshots generated by CI. Sorry that our infrastructure for this is currently lacking. Things involving snapshots are definitely much more painful than they need to be 😞 |
Replace new snapshots with some generated by CI
ghost
merged commit 
* develop: Update Num_Input to correct for Type Errors (#3074)
* develop: (165 commits) Add s4a message to close active modals (#2893) Add fail_on_warning to build options Docutils 0.16 Put back Sphinx to 3.0.3 Add import urllib to streamlit hello (#2969) (#3106) Update Num_Input to correct for Type Errors (#3074) Upgrade bokehjs to 2.2.2, and run `npx browserslist@latest --update-db` (#3090) remove extra star in docs (#3086) Remove flake8 linting tool (#3085) Makefile cleanup (#3083) Merge anchor headers feature branch into develop (#2983) Fix image galleries (#3044) Is this a red herring also? See if pylint was red herring Update sphinx Specify pylint version Fix pylint errors Added "allow-downloads" to the sandbox attributes (#3053) Small fix for `make pylint` command (#3062) Set genericColors properly and make theme defs more consistent (#3051) ...
3 participants
Possible solution to issue #2783.
Moved testing of types to above assignment of default value, and included value into the type test.
Altered assignment of default value to take into consideration of the types of all provided number arguments.
Tests still needed to be added/modified. Should tests be done in e2e, frontend, or both?