From 6750c7406e44056566a8de8625796783c363bf27 Mon Sep 17 00:00:00 2001 From: Giles Thompson Date: Fri, 10 Nov 2023 12:40:13 +1300 Subject: [PATCH 1/3] refactor(system upload): isolate onloadstart, onprogress, and onloadend for unit testing purposes --- ember-file-upload/src/internal.ts | 4 ++ ember-file-upload/src/system/upload.ts | 93 +++++++++++++++----------- 2 files changed, 57 insertions(+), 40 deletions(-) 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; From 8a7df717bc86f44f353c22bb7783cf0bf1a5b6e2 Mon Sep 17 00:00:00 2001 From: Giles Thompson Date: Fri, 10 Nov 2023 13:47:35 +1300 Subject: [PATCH 2/3] test(upload progress): encode originally reported events for unit testing --- .../tests/unit/system/upload-progress-test.ts | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 test-app/tests/unit/system/upload-progress-test.ts 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..4850884f --- /dev/null +++ b/test-app/tests/unit/system/upload-progress-test.ts @@ -0,0 +1,202 @@ +/* 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('Original (from 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); + }); + + test('Chrome (from 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); + }); + + test('Firefox (from 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); + }); +}); From 7fffdee09d56ea9a84afaea4dcd96ca6bc0083f0 Mon Sep 17 00:00:00 2001 From: Giles Thompson Date: Sat, 11 Nov 2023 10:40:03 +1300 Subject: [PATCH 3/3] test(upload progress): add assertion for final loadend with size of 0 Assert `uploadFile.size` after replaying events in all specs Improve documentation to highlight the purpose/source of each test --- .../tests/unit/system/upload-progress-test.ts | 89 ++++++++++++++++--- 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/test-app/tests/unit/system/upload-progress-test.ts b/test-app/tests/unit/system/upload-progress-test.ts index 4850884f..d92a9472 100644 --- a/test-app/tests/unit/system/upload-progress-test.ts +++ b/test-app/tests/unit/system/upload-progress-test.ts @@ -63,7 +63,58 @@ const replayEvents = ( module('Unit | upload progress', function (hooks) { setupTest(hooks); - test('Original (from issue #1002)', function (this: TestContext, assert) { + 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, @@ -109,9 +160,15 @@ module('Unit | upload progress', function (hooks) { ]; replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 248723, + 'upload filesize is set from handlers', + ); }); - test('Chrome (from issue #1002)', function (this: TestContext, assert) { + 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, @@ -153,9 +210,15 @@ module('Unit | upload progress', function (hooks) { ]; replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 618122, + 'upload filesize is set from handlers', + ); }); - test('Firefox (from issue #1002)', function (this: TestContext, assert) { + 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, @@ -176,27 +239,31 @@ module('Unit | upload progress', function (hooks) { loaded: 0, total: 618220, }), - assertProgressAfter: 0, }, { - assertProgressBefore: 0, event: new ProgressEvent('progress', { lengthComputable: true, - loaded: 30954, + loaded: 618220, total: 618220, }), - assertProgressAfter: 5.0069554527514475, + assertProgressAfter: 100, }, { - event: new ProgressEvent('progress', { + event: new ProgressEvent('loadend', { lengthComputable: true, - loaded: 62954, - total: 618220, + loaded: 0, + total: 0, }), - assertProgressAfter: 10.18310633754974, + assertProgressAfter: 100, }, ]; replayEvents(assert, uploadFile, steps); + + assert.strictEqual( + uploadFile.size, + 618220, + 'upload filesize is set from handlers', + ); }); });