Bug 87696 – [FileAPI] Miss FileList test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Li Yin]

The FileList related test cases are missed, it's necessary to add the attribute test in LayoutTests.

[Li Yin]

Created attachment 144455 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 144455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144455&action=review

Nice to see tests for this! (Drive-by comment, I'm not a reviewer)

> LayoutTests/fast/files/file-list-test.html:16
> +    debug("files.length: " + files.length);

Might be nicer to use shouldBe() family to make expected values clearer?

[Kentaro Hara]

Comment on attachment 144455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144455&action=review

The change looks OK.

> LayoutTests/fast/files/file-list-test.html:17
> +    debug("files.item(0): " + files.item(0));

Nit: Maybe we want to pass more than two files for testing?

> LayoutTests/fast/files/file-list-test.html:32
> +function runTests()
> +{
> +    eventSender.beginDragWithFiles(['resources/UTF8.txt']);
> +    eventSender.mouseMoveTo(10, 10);
> +    eventSender.mouseUp();
> +}
> +
> +if (window.eventSender) {
> +    layoutTestController.dumpAsText();
> +    window.onload = runTests;
> +}

I guess you can simplify the code.

- You do not need to call layoutTestController.dumpAsText().
- You can write the <script> after </body>, by which you can avoid hooking window.onload.

[Li Yin]

Created attachment 144477 [details]
Patch

[Li Yin]

(In reply to comment #2)
> (From update of attachment 144455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144455&action=review
> 
> Nice to see tests for this! (Drive-by comment, I'm not a reviewer)
> 
> > LayoutTests/fast/files/file-list-test.html:16
> > +    debug("files.length: " + files.length);
> 
> Might be nicer to use shouldBe() family to make expected values clearer?

Okay, thanks.

[Li Yin]

(In reply to comment #3)
> (From update of attachment 144455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144455&action=review
> 
> The change looks OK.
> 
> > LayoutTests/fast/files/file-list-test.html:17
> > +    debug("files.item(0): " + files.item(0));
> 
> Nit: Maybe we want to pass more than two files for testing?

Add two files in the List

> 
> > LayoutTests/fast/files/file-list-test.html:32
> > +function runTests()
> > +{
> > +    eventSender.beginDragWithFiles(['resources/UTF8.txt']);
> > +    eventSender.mouseMoveTo(10, 10);
> > +    eventSender.mouseUp();
> > +}
> > +
> > +if (window.eventSender) {
> > +    layoutTestController.dumpAsText();
> > +    window.onload = runTests;
> > +}
> 
> I guess you can simplify the code.
> 
> - You do not need to call layoutTestController.dumpAsText().
> - You can write the <script> after </body>, by which you can avoid hooking window.onload.

Simply the code, done.

Thanks for your review.

[Kentaro Hara]

Comment on attachment 144477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144477&action=review

> LayoutTests/fast/files/file-list-test.html:7
> +<input type="file" name="file" id="file" multiple onchange="onInputFileChange(this.files)">

Nit: name="file" and id="file" are not needed.

> LayoutTests/fast/files/file-list-test.html:18
> +    shouldBeTrue("files.item(0) instanceof File");

Also you can test files.item(0).name and files.item(1).name to check if the correct files are stored?

shouldBeEqualToString('files.item(0).name', 'resources/UTF8.txt');
shouldBeEqualToString('files.item(1).name', 'resources/UTF8-2.txt');

> LayoutTests/fast/files/file-list-test.html:20
> +    finishJSTest();

You can remove this.

[Li Yin]

Created attachment 144531 [details]
Patch

[Li Yin]

Created attachment 144532 [details]
Patch

[Li Yin]

(In reply to comment #7)
> (From update of attachment 144477 [details])
change="onInputFileChange(this.files)">
> 
> Nit: name="file" and id="file" are not needed.
> 

Done

> > LayoutTests/fast/files/file-list-test.html:18
> > +    shouldBeTrue("files.item(0) instanceof File");
> 
> Also you can test files.item(0).name and files.item(1).name to check if the correct files are stored?
> 
> shouldBeEqualToString('files.item(0).name', 'resources/UTF8.txt');
> shouldBeEqualToString('files.item(1).name', 'resources/UTF8-2.txt');
> 

Done.
But it should be shouldBeEqualToString('files.item(0).name', 'UTF8.txt');
File.name is merely the name of the file, without path information :) 

> > LayoutTests/fast/files/file-list-test.html:20
> > +    finishJSTest();
> 
> You can remove this.

Done

Thanks for your review.

[Kentaro Hara]

Comment on attachment 144532 [details]
Patch

OK!

[WebKit Review Bot]

Comment on attachment 144532 [details]
Patch

Clearing flags on attachment: 144532

Committed r118787: <http://trac.webkit.org/changeset/118787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jian Li]

Comment on attachment 144532 [details]
Patch

Thanks for adding test cases for FileList. Here're some minor comments that you could consider addressing them in another patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=144532&action=review

> LayoutTests/fast/files/file-list-test.html:22
> +    shouldBeNull("files.item(999)");

Could you please also add some more tests to cover the cases that 2, null, or undefined is passed as index.

Please also test the scenario that no file is selected.

[Alexey Proskuryakov]

EventSender.beginDragWithFiles is not supported by WebKitTestRunner. Skipped the test for WebKit2 in <http://trac.webkit.org/changeset/118825>.

[Li Yin]

(In reply to comment #14)
> (From update of attachment 144532 [details])
> Thanks for adding test cases for FileList. Here're some minor comments that you could consider addressing them in another patch.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=144532&action=review
> 
> > LayoutTests/fast/files/file-list-test.html:22
> > +    shouldBeNull("files.item(999)");
> 
> Could you please also add some more tests to cover the cases that 2, null, or undefined is passed as index.
>

There is a bug about the negative index, https://bugs.webkit.org/show_bug.cgi?id=77899
If we want to support negative index, I think the FileList.idl will be changed, and maybe the behavior will be different when the index is null, undefined.(I am not sure.)

So, currently, I only cover the expected index. And it will be improved in the bug77899.
 
> Please also test the scenario that no file is selected.