e2e test for file_uploader on_change bug #4390
Conversation
| @@ -56,3 +56,20 @@ | |||
| st.text("No upload") | |||
| else: | |||
| st.text(form_file.read()) | |||
|
|
|||
|
|
|||
| if st._is_running_with_streamlit: | |||
There was a problem hiding this comment.
I assume this if line is here because we run e2e tests in "bare Python mode" as part of our Python tests, and this test doesn't work in that circumstance? Can you add a comment here explaining that?
e2e/specs/st_file_uploader.spec.js
Outdated
| @@ -393,4 +393,51 @@ describe("st.file_uploader", () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| // regression test for https://github.com/streamlit/streamlit/issues/4256 bug | |||
| it("not calls callback if not changed", () => { | |||
There was a problem hiding this comment.
Nit: it("does not call a callback when not changed") instead?
|
|
||
| // On rerun, make sure callback is not called, since file not changed | ||
| cy.get("body").type("r"); | ||
| cy.wait(1000); |
There was a problem hiding this comment.
Not necessarily something to stop the PR for, but I wonder if there's a more deterministic way to wait for the rerun to happen than cy.wait(1000). Could we instead test for the presence of the "Running..." indicator?
There was a problem hiding this comment.
+1
The newly-added loadApp custom cypress command (in frontend/cypress/support/commands.js) has an example of how to do this. It sounds like it may be useful to generalize that into a a cypress command so that we have a standard way of waiting until an app has finished running
There was a problem hiding this comment.
Good!
I agree, having a more deterministic way of waiting for the application to load and using it is more correct and more reliable.
I'd like to mergde these changes as they are, as the cy.wait occurs in many places, and it would take a long time to rewrite it for me now.
And come back to using loadApp command in e2e tests at some point in the future!
Thank you for your comments!
|
|
||
| // On rerun, make sure callback is not called, since file not changed | ||
| cy.get("body").type("r"); | ||
| cy.wait(1000); |
There was a problem hiding this comment.
+1
The newly-added loadApp custom cypress command (in frontend/cypress/support/commands.js) has an example of how to do this. It sounds like it may be useful to generalize that into a a cypress command so that we have a standard way of waiting until an app has finished running
* develop: Bump url-parse from 1.5.3 to 1.5.7 in /frontend (#4414) ScriptRequestQueue cleanup (#4386) session_state docstrings (+ minor housekeeping) (#4403) Rename script_path to main_script_path (#4408) [STLT-17] ✨ (Release Pipeline) Add full release regression suite via pytest (#4397) [STLT-16] 🔄 (CircleCI Release Pipeline) Commit and push version changes (#4336) e2e test for file_uploader on_change bug (#4390) Add myself as a codeowner for new dependencies
📚 Context
Please describe the project or issue background here
What kind of change does this PR introduce?
🧠 Description of Changes
Add e2e test for 🎬 : Improved file uploader and camera input to call its on_change handler only when necessary #4270
Removed deprecated checks for Python 3.6 from
run_bare_integration_tests.pysince we support only Python >= 3.7🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.