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

AppTest format_func #8189

Merged

Conversation

AnOctopus
Copy link
Collaborator

Describe your changes

Fixes issues with selecting items in widgets when format_func is used, by storing the format_func in session state during app tests and using it to convert the provided values into the ones stored by the protobuf.

GitHub Issue Link (if applicable)

#7679 and #8019

Testing Plan

App tests added


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@AnOctopus AnOctopus marked this pull request as ready for review February 22, 2024 23:00
@@ -347,6 +347,8 @@ def _multiselect(
multiselect_proto.value[:] = serde.serialize(widget_state.value)
multiselect_proto.set_value = True

if ctx:
Copy link
Collaborator

Choose a reason for hiding this comment

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

since I am not too familiar with the testing framework yet: does this only resolve to True in the testing-scenario? If yes, it might make sense to leave a comment or rename ctx to make it more obvious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. It is true any time you are running streamlit. The only time the ScriptRunContext is not available is if you are running a streamlit script directly via python (python my_app.py)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! So this means that save_for_app_testing is executed also in non-testing flows, right? Will ctx.session_state[TESTING_KEY][k] always raise a key-error that is caught in non-testing executions then for every re-run? If yes, do we expect any performance issues because of it? My first reaction is that we would want to check for something like if IS_TEST_MODE: or similar

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be executed in all flows, and raise the error in non-testing executions. I don't expect it to be much of a performance cost, as exceptions are pretty similar to regular if branches in terms of perf in python.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would also feel slightly better if we could introduce some kind of global flag or config option to indicate that it is in app testing mode. We have something very similar for the development mode as well:

image

Having this flag could also be useful if we need to integrate other testing-related logic into Streamlit as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a config option

Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

lib/streamlit/config.py Outdated Show resolved Hide resolved
@AnOctopus AnOctopus enabled auto-merge (squash) March 7, 2024 18:55
@AnOctopus AnOctopus merged commit 2a6997b into streamlit:develop Mar 7, 2024
33 of 35 checks passed
@AnOctopus AnOctopus deleted the fix/app-test-selectbox-format-func branch March 7, 2024 19:16
AnOctopus added a commit that referenced this pull request Mar 9, 2024
…r in AppTest (#8264)

## Describe your changes
Reverts a previous fix that made trigger values correct in AppTest but
introduced an infinite recursion when `st.rerun()` is called
conditionally on the value of a trigger widget. Instead, making trigger
widgets return correct values in AppTest is handled by storing their
values at runtime using the new `save_for_app_testing` functionality
introduced by #8189

## GitHub Issue Link (if applicable)
#7768
OscarSaharoy added a commit to OscarSaharoy/streamlit that referenced this pull request Mar 13, 2024
passing arguments to CachedFunc.clear()

todo update
Reverts a previous fix that made trigger values correct in AppTest but
introduced an infinite recursion when `st.rerun()` is called
conditionally on the value of a trigger widget. Instead, making trigger
widgets return correct values in AppTest is handled by storing their
values at runtime using the new `save_for_app_testing` functionality
introduced by streamlit#8189

GitHub Issue Link
streamlit#8153
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
## Describe your changes
Fixes issues with selecting items in widgets when `format_func` is used,
by storing the `format_func` in session state during app tests and using
it to convert the provided values into the ones stored by the protobuf.

## GitHub Issue Link (if applicable)
streamlit#7679 and streamlit#8019 

## Testing Plan

App tests added
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
…r in AppTest (streamlit#8264)

## Describe your changes
Reverts a previous fix that made trigger values correct in AppTest but
introduced an infinite recursion when `st.rerun()` is called
conditionally on the value of a trigger widget. Instead, making trigger
widgets return correct values in AppTest is handled by storing their
values at runtime using the new `save_for_app_testing` functionality
introduced by streamlit#8189

## GitHub Issue Link (if applicable)
streamlit#7768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants