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

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski]

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.

Comment 1

[Thomas Wisniewski]

Attachment: 1286439-fix_the_last_FormData-append_XHR_WPT.diff

This patch addresses those issues, and it turns out that Firefox passes the corrected test.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 3

[Thomas Wisniewski]

Attachment: 1286439-fix_the_last_FormData-append_XHR_WPT.diff

Sure, I agree. Here's a version with those ideas incorporated.

Comment 4

[Thomas Wisniewski]

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.

Comment 5

[Anne (:annevk)]

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.

Comment 6

[James Graham [:jgraham]]

How sure are we that this won't have issues with clock resolution? Other than that it looks good.

Comment 7

[Thomas Wisniewski]

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).

Comment 8

[James Graham [:jgraham]]

I am not an expert, but I would be prepared to try that and see if it produces intermittent results in practice.

Comment 9

[Thomas Wisniewski]

Attachment: 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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10366f77d155
Fix the last FormData-append XHR web platform test. r=jgraham

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10366f77d155