Closed Bug 1286439 Opened 4 years ago Closed 4 years ago

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


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: wisniewskit, Assigned: wisniewskit)





(1 file, 2 obsolete files)

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.
This patch addresses those issues, and it turns out that Firefox passes the corrected test.
Assignee: nobody → wisniewskit
Attachment #8770380 - Flags: review?(annevk)
Comment on attachment 8770380 [details] [diff] [review]

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(,;
> +        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 before and after create_formdata, and checking if lastModified is between the before and after values.
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)
Comment on attachment 8770626 [details] [diff] [review]

: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)
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+
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)
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
Keywords: checkin-needed
Pushed by
Fix the last FormData-append XHR web platform test. r=jgraham
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.