-
Notifications
You must be signed in to change notification settings - Fork 26
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
Bug 1677440 - Implement a browser upload adapter #25
Conversation
tests/upload/adapter.spec.ts
Outdated
@@ -2,14 +2,49 @@ | |||
* License, v. 2.0. If a copy of the MPL was not distributed with this | |||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
|
|||
import "jsdom-global/register"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I literally only have this because it includes DOMException
. One less thing for me to mock.
tests/upload/adapter.spec.ts
Outdated
|
||
const sandbox = sinon.createSandbox(); | ||
|
||
global.fetch = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this code is hacky AF. But the bug I opened to look into mocking fetch is what should fix it.
2cfb4fa
to
f93a594
Compare
export interface UploadAdapter { | ||
export abstract class Uploader { | ||
// The timeout, in seconds, to use for all operations with the server. | ||
protected defaultTimeout = 10_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this from the python bindings, on the android bindings we have two different timeouts: one for connection timeout and another for request timeout. I don't know that there is a way to make this differentiation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to not have the differentiation if the uploader doesn't allow it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc
export interface UploadAdapter { | ||
export abstract class Uploader { | ||
// The timeout, in seconds, to use for all operations with the server. | ||
protected defaultTimeout = 10_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to not have the differentiation if the uploader doesn't allow it :)
Adding tests for the timeout mechanism and cookie removal is blocked because we need to figure out the mest way bot to mock fetch on these tests.
Filed a bug to do that later, for now I manually tested and it "works on my machine"™.