Add a pause for file uploading E2E test #2438
Conversation
e2e/specs/st_file_uploader.spec.js
Outdated
| }) | ||
| }); | ||
| // Give some time for the first file to upload | ||
| cy.wait(500); |
There was a problem hiding this comment.
Hm, I expect this will mostly work, but two concerns:
- It could still flake, it's just a lot less likely to based on upload speed?
- This adds an extra half-second to our CircleCI run always, since our CircleCI tests are currently run sequentially
Could we instead sort the strings as Tim suggested, to resolve these issues?
There was a problem hiding this comment.
If you expand below, this is supposed to test the order the files are uploaded. I'm uploading them in a specific order and then I'm verifying the order below so it is manually sorted
There was a problem hiding this comment.
Is it important to test the order that files are uploaded? I'm also skeptical of using timers to mitigate non-deterministic tests.
A possibly more-correct way to test this would be to not upload the second file until the server indicates it's received the first file - possibly by changing some text on the page or something. Or maybe we need to add "uploaded at" timestamp values to UploadedFileRec, so that the test can sort by timestamp. But I wonder if this something we actually care about testing; I don't think make we any claims in the API about the ordering of multiple files, and I'm not sure what the use case would be?
There was a problem hiding this comment.
We don't make any claims about the sorting order so we don't have to test this but I do believe this is desirable behavior. Preserving this sorting allows streamlit developers to provide a better UX to their users. For example if someone uploads 2 files and each file results in one table being printed, when a 3rd file is uploaded, the table for the new file should be printed after the first 2. In order to do this, we would have to preserve the order of the list so that the python script would process the files in the same order on rerun.
We could add an uploaded at timestamp but sounds like that would be solely for the purposes of this test case? I think the waiting to upload the second file until we have received the first is the correct way to go instead and will update.
karriebear
force-pushed
the
file-uploader-multiple-uploads-test
branch
2 times, most recently
from
3f119ca to
8471c25
Compare
karriebear
force-pushed
the
file-uploader-multiple-uploads-test
branch
from
8471c25 to
386da42
Compare
| events: ["dragenter", "drop"] | ||
| }); | ||
|
|
||
| cy.wait("@uploadFile"); |
There was a problem hiding this comment.
Hm, I think I'm missing something -- why are we calling cy.wait after the second file is uploaded, as opposed to before?
(In the previous version of the code, cy.wait(500) was before the second upload, which made more intuitive sense to me. But maybe the goal of the wait is different this time?)
There was a problem hiding this comment.
Before we were calling a random wait but now we added the assertion to check if one file has been uploaded. This in itself comes with a built in wait for the uploaded file to become available. However, since this element is already available, when we do another check after the 2nd upload, it doesn't wait for the request to finish so here we are waiting for the HTTP request to finish. Tried to check for the progress bar showing up but at least on my machine, the file gets uploaded too quickly for the progress bar to get rendered. Also waiting for the request to finish seems more legit
There was a problem hiding this comment.
Ah, makes sense! Thanks for the explanation. Could we capture this briefly as code comments?
akrolsmir
left a comment
akrolsmir
left a comment
There was a problem hiding this comment.
Thanks again for fixing this up! Deflaking our tests buys us back the eng time waiting for CircleCI, it's a great investment in making our developer experience buttery-smooth \o/
| events: ["dragenter", "drop"] | ||
| }); | ||
|
|
||
| cy.wait("@uploadFile"); |
There was a problem hiding this comment.
Ah, makes sense! Thanks for the explanation. Could we capture this briefly as code comments?
3 participants
Our E2E test is reversing the order of the files uploaded intermittently. This could be happening too fast in certain runs. Adding a pause between file uploads.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.