From afd70daa7219e6680dc9350c48a69e34493794b1 Mon Sep 17 00:00:00 2001 From: Giles Thompson Date: Sat, 11 Nov 2023 10:47:43 +1300 Subject: [PATCH] Replay progress events in unit tests (#1017) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ember-file-upload/src/internal.ts | 4 + ember-file-upload/src/system/upload.ts | 93 +++--- .../tests/unit/system/upload-progress-test.ts | 269 ++++++++++++++++++ 3 files changed, 326 insertions(+), 40 deletions(-) create mode 100644 test-app/tests/unit/system/upload-progress-test.ts diff --git a/ember-file-upload/src/internal.ts b/ember-file-upload/src/internal.ts index 1ea51140..0e24cd27 100644 --- a/ember-file-upload/src/internal.ts +++ b/ember-file-upload/src/internal.ts @@ -1,10 +1,14 @@ import DataTransferWrapper from './system/data-transfer-wrapper.ts'; import HTTPRequest from './system/http-request.ts'; import UploadFileReader from './system/upload-file-reader.ts'; +import { onloadstart, onprogress, onloadend } from './system/upload.ts'; export { // Non-public classes imported by the test app DataTransferWrapper, HTTPRequest, UploadFileReader, + onloadstart, + onprogress, + onloadend, }; diff --git a/ember-file-upload/src/system/upload.ts b/ember-file-upload/src/system/upload.ts index 64d6dfb3..8ee95289 100644 --- a/ember-file-upload/src/system/upload.ts +++ b/ember-file-upload/src/system/upload.ts @@ -52,6 +52,56 @@ function normalizeOptions( return options; } +export function onloadstart( + file: UploadFile, + event?: ProgressEvent, +) { + if (!event) return; + if (!event.lengthComputable || event.total === 0) return; + + file.loaded = event.loaded; + // It occurs that the event.total is not always correct. + // For this reason we should hold the max file size. + // The correct should be returned while progress + file.size = Math.max(file.size, event.total); + file.progress = (file.loaded / file.size) * 100; +} + +export function onprogress( + file: UploadFile, + event?: ProgressEvent, +) { + if (!event) return; + // We need to check also for isUploadComplete, because the browsers brings sometimes the onprogress after onloadend event + if (!event.lengthComputable || event.total === 0 || file.isUploadComplete) + return; + + file.size = event.total; + + // When the progress is completed there is possible that we get the `Content-Length` response header of the upload endpoint as loaded / total. + // There is possible that `Content-Length` is lower or higher than the already loaded bytes. + // if there is lower, we want to keep the higher loaded value, otherwise the progress percentage will be decreased + // When the event.loaded is higher than the start file.size, we use the file.size, otherwise it can occur that progress for the file is higher than 100% + let loaded = event.loaded; + if (loaded > file.size) { + loaded = file.size; + } + file.loaded = Math.max(loaded, file.loaded); + file.progress = (file.loaded / file.size) * 100; +} + +export function onloadend( + file: UploadFile, + event?: ProgressEvent, +) { + if (!event) return; + if (!event.lengthComputable || event.total === 0) return; + + file.loaded = file.size; + file.progress = (file.loaded / file.size) * 100; + file.isUploadComplete = true; +} + export function upload( file: UploadFile, url: string | object, @@ -87,46 +137,9 @@ export function upload( request.timeout = options.timeout; } - request.onloadstart = function (evt) { - if (!evt) return; - if (!evt.lengthComputable || evt.total === 0) return; - - file.loaded = evt.loaded; - // It occurs that the evt.total is not always correct. - // For this reason we should hold the max file size. - // The correct should be returned while progress - file.size = Math.max(file.size, evt.total); - file.progress = (file.loaded / file.size) * 100; - }; - - request.onprogress = function (evt) { - if (!evt) return; - // We need to check also for isUploadComplete, because the browsers brings sometimes the onprogress after onloadend event - if (!evt.lengthComputable || evt.total === 0 || file.isUploadComplete) - return; - - file.size = evt.total; - - // When the progress is completed there is possible that we get the `Content-Length` response header of the upload endpoint as loaded / total. - // There is possible that `Content-Length` is lower or higher than the already loaded bytes. - // if there is lower, we want to keep the higher loaded value, otherwise the progress percentage will be decreased - // When the evt.loaded is higher than the start file.size, we use the file.size, otherwise it can occur that progress for the file is higher than 100% - let loaded = evt.loaded; - if (loaded > file.size) { - loaded = file.size; - } - file.loaded = Math.max(loaded, file.loaded); - file.progress = (file.loaded / file.size) * 100; - }; - - request.onloadend = function (evt) { - if (!evt) return; - if (!evt.lengthComputable || evt.total === 0) return; - - file.loaded = file.size; - file.progress = (file.loaded / file.size) * 100; - file.isUploadComplete = true; - }; + request.onloadstart = (event) => onloadstart(file, event); + request.onprogress = (event) => onprogress(file, event); + request.onloadend = (event) => onloadend(file, event); request.ontimeout = () => { file.state = FileState.TimedOut; diff --git a/test-app/tests/unit/system/upload-progress-test.ts b/test-app/tests/unit/system/upload-progress-test.ts new file mode 100644 index 00000000..d92a9472 --- /dev/null +++ b/test-app/tests/unit/system/upload-progress-test.ts @@ -0,0 +1,269 @@ +/* eslint-disable qunit/no-conditional-assertions */ + +import { module, test } from 'qunit'; +import { setupTest } from 'ember-qunit'; +import { FileSource, UploadFile } from 'ember-file-upload'; +import { type TestContext } from '@ember/test-helpers'; +import { png } from 'test-app/tests/utils/file-data'; +import { onloadstart, onprogress, onloadend } from 'ember-file-upload/internal'; + +interface Handlers { + [key: string]: typeof onloadstart; +} + +const handlers: Handlers = { + loadstart: onloadstart, + progress: onprogress, + loadend: onloadend, +}; + +type Step = { + assertProgressBefore?: number; + event: ProgressEvent; + assertProgressAfter?: number; +}; + +const replayEvents = ( + assert: Assert, + uploadFile: UploadFile, + steps: Step[], +) => { + for (const step of steps) { + if (typeof step.assertProgressBefore === 'number') { + assert.strictEqual( + uploadFile.progress, + step.assertProgressBefore, + `before (index: ${steps.indexOf(step)}), progress is: ${ + step.assertProgressBefore + }% type: ${step.event.type}, total: ${step.event.total}, loaded: ${ + step.event.loaded + }`, + ); + } + + const handler = handlers[step.event.type]; + if (!handler) throw new Error(`No handler matched ${step.event.type}`); + + handler(uploadFile, step.event); + + if (typeof step.assertProgressAfter === 'number') { + assert.strictEqual( + uploadFile.progress, + step.assertProgressAfter, + `after (index: ${steps.indexOf(step)}), progress is: ${ + step.assertProgressAfter + }% type: ${step.event.type}, total: ${step.event.total}, loaded: ${ + step.event.loaded + }`, + ); + } + } +}; + +module('Unit | upload progress', function (hooks) { + setupTest(hooks); + + test('nominal progress (recorded from Firefox in issue #1002)', function (this: TestContext, assert) { + const uploadFile = new UploadFile( + new File(png, 'test-filename.png'), + FileSource.Browse, + ); + + const steps = [ + { + assertProgressBefore: 0, + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 0, + }), + }, + { + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 618220, + }), + assertProgressAfter: 0, + }, + { + assertProgressBefore: 0, + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 30954, + total: 618220, + }), + assertProgressAfter: 5.0069554527514475, + }, + { + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 62954, + total: 618220, + }), + assertProgressAfter: 10.18310633754974, + }, + ]; + + replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 618220, + 'upload filesize is set from handlers', + ); + }); + + test('regression: content length of final `progress` and `loadend` is less than file size (Issue #1002)', function (this: TestContext, assert) { + const uploadFile = new UploadFile( + new File(png, 'test-filename.png'), + FileSource.Browse, + ); + + const steps = [ + { + assertProgressBefore: 0, + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 248723, + total: 248723, + }), + assertProgressAfter: 100, + }, + { + assertProgressBefore: 100, + event: new ProgressEvent('loadend', { + lengthComputable: true, + loaded: 248723, + total: 248723, + }), + assertProgressAfter: 100, + }, + { + assertProgressBefore: 100, + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 390, + total: 390, + }), + assertProgressAfter: 100, + }, + { + assertProgressBefore: 100, + event: new ProgressEvent('loadend', { + lengthComputable: true, + loaded: 390, + total: 390, + }), + assertProgressAfter: 100, + }, + ]; + + replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 248723, + 'upload filesize is set from handlers', + ); + }); + + test('regression: incorrect `total` in second `loadstart` event (recorded from Google Chrome in issue #1002)', function (this: TestContext, assert) { + const uploadFile = new UploadFile( + new File(png, 'test-filename.png'), + FileSource.Browse, + ); + + const steps = [ + { + assertProgressBefore: 0, + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 0, + }), + }, + { + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 712, + }), + }, + { + assertProgressBefore: 0, + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 16384, + total: 618122, + }), + assertProgressAfter: 2.6506094266180464, + }, + { + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 32768, + total: 618122, + }), + assertProgressAfter: 5.301218853236093, + }, + ]; + + replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 618122, + 'upload filesize is set from handlers', + ); + }); + + test('final response has `Content-Length: 0` (loadend with total: 0)', function (this: TestContext, assert) { + const uploadFile = new UploadFile( + new File(png, 'test-filename.png'), + FileSource.Browse, + ); + + const steps = [ + { + assertProgressBefore: 0, + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 0, + }), + }, + { + event: new ProgressEvent('loadstart', { + lengthComputable: true, + loaded: 0, + total: 618220, + }), + }, + { + event: new ProgressEvent('progress', { + lengthComputable: true, + loaded: 618220, + total: 618220, + }), + assertProgressAfter: 100, + }, + { + event: new ProgressEvent('loadend', { + lengthComputable: true, + loaded: 0, + total: 0, + }), + assertProgressAfter: 100, + }, + ]; + + replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 618220, + 'upload filesize is set from handlers', + ); + }); +});