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

Replay progress events in unit tests #1017

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Replay progress events in unit tests #1017

merged 3 commits into from
Nov 10, 2023

Conversation

gilest
Copy link
Collaborator

@gilest gilest commented Nov 10, 2023

Skips the browser, http, network etc. and lets us test the smallest unit of our internals – an UploadFile having its state updated by a series of ProgressEvent events.

For each scenario I'm pretty sure we only need to test a single file with a stream of synchronous events.

Covers some recorded events from different browsers and endpoints. We can add more scenarios here if we come across them.

Specifically, the Chrome scenario proves the fix in #1016 – this was previously failing early with 100% progress, which is the same issue reported by users in #1013

I've painstakingly copied values from log screenshots to setup this existing suite. Hopefully contributors can PR full tests in future to make things a bit easier 😅

Fixes #1013

image

@gilest gilest changed the title Unit test coverage for upload progress events Unit test scenarios for upload progress events Nov 10, 2023
@RobbieTheWagner
Copy link
Member

Thanks for the quick turnaround on this @gilest! Excited to see this come together 😄

@@ -0,0 +1,202 @@
/* eslint-disable qunit/no-conditional-assertions */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if there's a good way to avoid this 🤔

The data structure and complexity is a bit high for assert.step and assert.verifySteps.

I'm pretty confident that replayEvents is simple enough that assertions are running when we expect them to.

@gilest gilest requested a review from jelhan November 10, 2023 01:27
@gilest
Copy link
Collaborator Author

gilest commented Nov 10, 2023

@mkszepp please review ☝🏻

@mkszepp
Copy link
Contributor

mkszepp commented Nov 10, 2023

@gilest looks good, added two comments in code...
We should also add a test in which loadend brings total: 0, because when sombody returns only status 2xx with no content it occurs

Assert `uploadFile.size` after replaying events in all specs

Improve documentation to highlight the purpose/source of each test
@gilest
Copy link
Collaborator Author

gilest commented Nov 10, 2023

We should also add a test in which loadend brings total: 0, because when sombody returns only status 2xx with no content it occurs

Ah yes – great idea. I have added a test for this.

@gilest looks good, added two comments in code...

Thanks – not sure if your code comments got saved, I cannot see any...

I'm going to merge and release but feel free to open a tidy-up PR with your suggestions

@gilest gilest changed the title Unit test scenarios for upload progress events Replay progress events in unit tests Nov 10, 2023
@gilest gilest merged commit afd70da into master Nov 10, 2023
14 checks passed
@gilest gilest deleted the test/upload-events branch November 10, 2023 21:47
gilest pushed a commit that referenced this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progress goes straight to 100% when starting a file upload
3 participants