[XHR2] Final test in FormData-append.html seems incorrect.

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Thomas Wisniewski, Assigned: Thomas Wisniewski)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The last test in the FormData-append.html web platform test seems off. For starters it should be passing an array, not just a Blob, into the File constructor.

In addition, it's never going to match the two File objects by simply doing an assert_object_equals, as the lastModified property on both objects will never precisely match.
(Assignee)

Comment 1

2 years ago
Created attachment 8770380 [details] [diff] [review]
1286439-fix_the_last_FormData-append_XHR_WPT.diff

This patch addresses those issues, and it turns out that Firefox passes the corrected test.
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8770380 - Flags: review?(annevk)
Comment on attachment 8770380 [details] [diff] [review]
1286439-fix_the_last_FormData-append_XHR_WPT.diff

Review of attachment 8770380 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with these changes

::: testing/web-platform/tests/XMLHttpRequest/FormData-append.html
@@ +80,5 @@
>          assert_equals(fd.get('key'), "null");
>      }, 'testFormDataAppendToFormNull2');
>      test(function() {
> +        var a = create_formdata(['key', new Blob(), 'blank.txt']).get('key');
> +        var b = new File([new Blob()], 'blank.txt');

Since this file is testing formdata, not the File constructor, maybe we should just test the formdata's properties directly?

@@ +84,5 @@
> +        var b = new File([new Blob()], 'blank.txt');
> +        assert_equals(a.name, b.name);
> +        assert_equals(a.type, b.type);
> +        assert_equals(a.size, b.size);
> +        assert_less_than(b.lastModified - a.lastModified, 1000); // should be close

There is a danger of intermittent failures here... I would instead suggest calling Date.now() before and after create_formdata, and checking if lastModified is between the before and after values.
(Assignee)

Comment 3

2 years ago
Created attachment 8770626 [details] [diff] [review]
1286439-fix_the_last_FormData-append_XHR_WPT.diff

Sure, I agree. Here's a version with those ideas incorporated.
Attachment #8770380 - Attachment is obsolete: true
Attachment #8770380 - Flags: review?(annevk)
Attachment #8770626 - Flags: review?(annevk)
(Assignee)

Comment 4

2 years ago
Comment on attachment 8770626 [details] [diff] [review]
1286439-fix_the_last_FormData-append_XHR_WPT.diff

:annevk is on vacation until next week, so I'll change the review request to :jgraham for now.
Attachment #8770626 - Flags: review?(annevk) → review?(james)

Comment 5

2 years ago
This looks good. The fact that it's a File object is covered by the name property getter. Feel free to land. Note that Ms2ger is also a WPT reviewer.
How sure are we that this won't have issues with clock resolution? Other than that it looks good.
Attachment #8770626 - Flags: review?(james) → review+
(Assignee)

Comment 7

2 years ago
Funny enough, you raised that concern just as I got feedback on bug 1283720, which revealed that such timing issues will happen from time to time (the test there ran into a one-second drift on our try servers).

I'd imagine it would be fine to just use "now minus 2 seconds" as the before date in this test, to compensate for such potential skew. Do you agree? (I'd do the same in the other bug too, since the issues are similar).
Flags: needinfo?(james)
I am not an expert, but I would be prepared to try that and see if it produces intermittent results in practice.
Flags: needinfo?(james)
(Assignee)

Comment 9

2 years ago
Created attachment 8772471 [details] [diff] [review]
1286439-fix_the_last_FormData-append_XHR_WPT.diff

Alright, I've made that adjustment to the patch (so that it uses "two seconds ago" as its before-value). I also similarly drive-by adjusted the test causing bug 1283720, since it was running into the same issue.

Carrying over r+ and requesting checkin.
Attachment #8770626 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10366f77d155
Fix the last FormData-append XHR web platform test. r=jgraham
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10366f77d155
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.