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

e2e test for file_uploader on_change bug #4390

Merged
merged 5 commits into from Feb 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions e2e/scripts/st_file_uploader.py
Expand Up @@ -20,6 +20,10 @@
else:
st.text(single_file.read())

# Here and throughout this file, we use if st._is_running_with_streamlit:
# since we also run e2e python files in "bare Python mode" as part of our
# Python tests, and this doesn't work in that circumstance
# st.session_state can only be accessed while running with streamlit
if st._is_running_with_streamlit:
st.write(repr(st.session_state.single) == repr(single_file))

Expand Down Expand Up @@ -56,3 +60,20 @@
st.text("No upload")
else:
st.text(form_file.read())


if st._is_running_with_streamlit:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

if not st.session_state.get("counter"):
st.session_state["counter"] = 0

def file_uploader_on_change():
st.session_state.counter += 1

st.file_uploader(
"Drop a file:",
type=["txt"],
key="on_change_file_uploader_key",
on_change=file_uploader_on_change,
)

st.text(st.session_state.counter)
47 changes: 47 additions & 0 deletions e2e/specs/st_file_uploader.spec.js
Expand Up @@ -393,4 +393,51 @@ describe("st.file_uploader", () => {
);
});
});

// regression test for https://github.com/streamlit/streamlit/issues/4256 bug
it("does not call a callback when not changed", () => {
const fileName1 = "file1.txt";
const uploaderIndex = 4;

cy.fixture(fileName1).then(file1 => {
const files = [
{ fileContent: file1, fileName: fileName1, mimeType: "text/plain" }
];

// Script contains counter variable stored in session_state with
// default value 0. We increment counter inside file_uploader callback
// Since callback did not called at this moment, counter value should
// be equal 0
cy.getIndexed("[data-testid='stText']", uploaderIndex).should(
"contain.text",
"0"
);

// Uploading file, should invoke on_change call and counter increment
cy.getIndexed(
"[data-testid='stFileUploadDropzone']",
uploaderIndex
).attachFile(files[0], {
force: true,
subjectType: "drag-n-drop",
events: ["dragenter", "drop"]
});

// Make sure callback called
cy.getIndexed("[data-testid='stText']", uploaderIndex).should(
"contain.text",
"1"
);

// On rerun, make sure callback is not called, since file not changed
cy.get("body").type("r");
cy.wait(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!


// Counter should be still equal 1
cy.getIndexed("[data-testid='stText']", uploaderIndex).should(
"contain.text",
"1"
);
});
});
});
7 changes: 0 additions & 7 deletions scripts/run_bare_integration_tests.py
Expand Up @@ -29,9 +29,6 @@
from typing import Set

import click
import matplotlib

IS_PYTHON_3_6 = sys.version_info[:2] == (3, 6)

# Where we expect to find the example files.
E2E_DIR = "e2e/scripts"
Expand All @@ -47,10 +44,6 @@
# that doesn't require a display.
os.environ["MPLBACKEND"] = "Agg"

# magic.py uses contextlib.asynccontextmanager, which is Python 3.7+
if IS_PYTHON_3_6:
EXCLUDED_FILENAMES.add("st_magic.py")


def _command_to_string(command):
if isinstance(command, list):
Expand Down