Bug 44920 - Add layout tests for FileSystem API
: Add layout tests for FileSystem API
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 42903
  Show dependency treegraph
 
Reported: 2010-08-30 18:33 PST by
Modified: 2010-10-07 14:10 PST (History)


Attachments
Patch (70.20 KB, patch)
2010-09-16 17:22 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.28 KB, patch)
2010-09-17 18:17 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (69.58 KB, patch)
2010-09-17 18:20 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (86.02 KB, patch)
2010-09-29 13:42 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (77.91 KB, patch)
2010-09-29 20:15 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.01 KB, patch)
2010-09-29 20:21 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (70.03 KB, patch)
2010-10-04 01:48 PST, Kinuko Yasuda
levin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-30 18:33:31 PST
FileSystem API needs layout tests.
------- Comment #1 From 2010-09-16 17:22:35 PST -------
Created an attachment (id=67866) [details]
Patch
------- Comment #2 From 2010-09-16 22:56:30 PST -------
(From update of attachment 67866 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=67866&action=prettypatch

> LayoutTests/fast/filesystem/op-copy.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

don't think we need this line. most layout tests i've seen don't have it.

> LayoutTests/fast/filesystem/op-copy.html:11
> +<script type="application/file-system-test-shell" id="file-system-test-script">

i'm not familiar with this type/format. can you please point me to some relevant docs/classes/descriptions?
------- Comment #3 From 2010-09-17 00:36:06 PST -------
(In reply to comment #2)
> (From update of attachment 67866 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67866&action=prettypatch
> 
> > LayoutTests/fast/filesystem/op-copy.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> don't think we need this line. most layout tests i've seen don't have it.

Will remove.

> > LayoutTests/fast/filesystem/op-copy.html:11
> > +<script type="application/file-system-test-shell" id="file-system-test-script">
> 
> i'm not familiar with this type/format. can you please point me to some relevant docs/classes/descriptions?

(Ah I forgot to prefix it with "x-"...)  It's just one of "application/x-our-own-format" types.
The intention was: to have the test script (fs-test-shell.js) read the content of a given element and executes line by line for testing.  (I can replace <script> tag with <pre> or something looking more innocent if it could be better.)
------- Comment #4 From 2010-09-17 18:17:24 PST -------
Created an attachment (id=67986) [details]
Patch
------- Comment #5 From 2010-09-17 18:20:35 PST -------
Created an attachment (id=67987) [details]
Patch
------- Comment #6 From 2010-09-17 18:26:00 PST -------
(In reply to comment #2)
> (From update of attachment 67866 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67866&action=prettypatch
> 
> > LayoutTests/fast/filesystem/op-copy.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> don't think we need this line. most layout tests i've seen don't have it.

Deleted.

> > LayoutTests/fast/filesystem/op-copy.html:11
> > +<script type="application/file-system-test-shell" id="file-system-test-script">
> 
> i'm not familiar with this type/format. can you please point me to some relevant docs/classes/descriptions?

As in my previous comment it's a custom type just for testing.
Changed the type to "application/x-file-system-test-shell" hoping it makes less confusion.
------- Comment #7 From 2010-09-21 15:06:03 PST -------
(From update of attachment 67987 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=67987&action=review

All the results generally look like "8" is "8"... can these have a little more context in them. right now they're impossible for a human to parse

tests should be as such

shouldBe("controller.operation.result", "true");

so the results are understandable when something breaks

> LayoutTests/fast/filesystem/script-tests/simple-persistent.js:13
> +    shouldBeTrue("true");

this looks meaningless

> LayoutTests/fast/filesystem/simple-temporary-expected.txt:7
> +WARN: shouldBe() expects string arguments

this looks wrong
------- Comment #8 From 2010-09-29 13:42:41 PST -------
Created an attachment (id=69245) [details]
Patch
------- Comment #9 From 2010-09-29 13:46:12 PST -------
(In reply to comment #7)
> (From update of attachment 67987 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67987&action=review
> 
> All the results generally look like "8" is "8"... can these have a little more context in them. right now they're impossible for a human to parse
> tests should be as such
> shouldBe("controller.operation.result", "true");
> so the results are understandable when something breaks

Updated the tests and expectations to make them contain more informative messages.

> > LayoutTests/fast/filesystem/script-tests/simple-persistent.js:13
> > +    shouldBeTrue("true");
> this looks meaningless

Fixed.

> > LayoutTests/fast/filesystem/simple-temporary-expected.txt:7
> > +WARN: shouldBe() expects string arguments
> this looks wrong

Fixed.
------- Comment #10 From 2010-09-29 20:15:50 PST -------
Created an attachment (id=69304) [details]
Patch

Rebased.
------- Comment #11 From 2010-09-29 20:21:59 PST -------
Created an attachment (id=69306) [details]
Patch
------- Comment #12 From 2010-10-01 13:27:06 PST -------
(From update of attachment 69306 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69306&action=review

> LayoutTests/fast/filesystem/resources/fs-test-shell.js:1
> +/*

You're defining a whole new scripting language here in order to write filesystem tests.
1) Is this absolutely necessary?  The language looks very much like JavaScript...could it go more in that direction?  Can it be a JS library instead of an interpreter, as with e.g. http://jsmock.sourceforge.net/examples/, http://pivotal.github.com/jasmine/?  Even if it's just a set of JavaScript functions that do the same thing as your script functions, at least other folks editing the tests don't have to learn a new syntax, and can mix in new JS code without having to extend your interpreter.
2) Can we just use an existing library?
3) If we decide to go with a new domain-specific language, it should really be of general utility, and not buried down under the filesystem tests.  Lots of tests could make use of it.
4) There's enough code here in your interpreter that it really needs its own unit tests.

> LayoutTests/fast/filesystem/resources/fs-test-shell.js:19
> +    entry3 = ROOT.getFile('foo/nonexistent') raises 8

Is it raise or raises?

> LayoutTests/fast/filesystem/resources/fs-test-shell.js:220
> +        this.log('Reseting the filesystem...');

typo: Resetting [affects expectations].
------- Comment #13 From 2010-10-01 15:08:54 PST -------
(In reply to comment #12)
> (From update of attachment 69306 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69306&action=review
> 
> > LayoutTests/fast/filesystem/resources/fs-test-shell.js:1
> > +/*
> 
> You're defining a whole new scripting language here in order to write filesystem tests.
> 1) Is this absolutely necessary?  The language looks very much like JavaScript...could it go more in that direction?  Can it be a JS library instead of an interpreter, as with e.g. http://jsmock.sourceforge.net/examples/, http://pivotal.github.com/jasmine/?  Even if it's just a set of JavaScript functions that do the same thing as your script functions, at least other folks editing the tests don't have to learn a new syntax, and can mix in new JS code without having to extend your interpreter.

Thanks very much for your comments.
Let me think about it, but to me this was the most straightforward way to write and debug so many test cases in short time.  And I wanted have something that can be easily extended for Sync cases.

(Of course we can do the same set of tests in pure JS code.  I wrote some tests in that way.)

> 2) Can we just use an existing library?
> 3) If we decide to go with a new domain-specific language, it should really be of general utility, and not buried down under the filesystem tests.  Lots of tests could make use of it.
> 4) There's enough code here in your interpreter that it really needs its own unit tests.

I don't think we want to do 2) or 3) because of the reason 4).  (And it's DSL because it's specific to FileSystem.)  Btw the interpreter part is basically just regexp's and it's less than 100 lines.

> > LayoutTests/fast/filesystem/resources/fs-test-shell.js:19
> > +    entry3 = ROOT.getFile('foo/nonexistent') raises 8
> 
> Is it raise or raises?

Both work...

> > LayoutTests/fast/filesystem/resources/fs-test-shell.js:220
> > +        this.log('Reseting the filesystem...');
> 
> typo: Resetting [affects expectations].
------- Comment #14 From 2010-10-01 16:27:01 PST -------
(From update of attachment 69306 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69306&action=review

> LayoutTests/fast/filesystem/op-get-entry.html:37
> +    ASSERT_EQ('/a/c', dir1_file3.fullPath)

Why hard-code the strings here, when you're using variables for the names?
My reflex would be to construct the result strings via concatenation of the names, but you could also just pass the raw strings into getDirectory and getFile instead, so you could see at a glance that the inputs + outputs match.

> LayoutTests/fast/filesystem/op-get-entry.html:69
> +    dir = ROOT.getDirectory('/parent/a/a', create_flag)

By this point in the code I've forgotten that "name1" means "a" ;'>.

> LayoutTests/fast/filesystem/op-get-entry.html:74
> +    # The entry's path must not be used if the given path is a full path.

I'm not sure what this comment means.

> LayoutTests/fast/filesystem/op-get-entry.html:80
> +    print('* Getting files/directories with relative paths.')

Add some relative path tests with "." and "..", e.g. "/parent/a/a/../../a/d", "/parent/a/./././../././a/d".
Also make sure that we can't reach out of the sandbox by verifying that e.g. "/../../../../../../../a" == "/a".

> LayoutTests/fast/filesystem/op-move.html:35
> +    parent.getDirectory(name3)

What's this last getDirectory(name3) for?  Should that be getFile(name4)?

> LayoutTests/fast/filesystem/resources/fs-test-shell-temporary.js:1
> +function endTest() {

Is this wrapper function needed?

> LayoutTests/fast/filesystem/restricted-chars.html:13
> +

You need to pass in the create flag for all of these.  Also, it would be nice to add one creating 'ab' that succeeds.

> LayoutTests/fast/filesystem/restricted-chars.html:14
> +    ROOT.getFile('a\\b') raises 13

Also try getFile('a/b'), since there's no directory 'a'.

> LayoutTests/fast/filesystem/restricted-names.html:58
> +    file.copyTo(dir, 'foo.') raises 13

Test moveTo as well.

> LayoutTests/fast/filesystem/script-tests/parallel-operations.js:53
> +    helper.run(function() { b.moveTo(fileSystem.root, 'b2', done, errorCallback); });

You're copying a to b, but you're also moving b.  Is that a race condition?
------- Comment #15 From 2010-10-01 16:46:58 PST -------
Great thanks.

(In reply to comment #14)
> (From update of attachment 69306 [details] [details])
> > LayoutTests/fast/filesystem/op-get-entry.html:74
> > +    # The entry's path must not be used if the given path is a full path.
> 
> I'm not sure what this comment means.

If the given path is a relative path we construct a new path by concatenating entry.fullPath, the given path and the new name.  Otherwise (== the given path is an absolute path) we'll just use the given path.

... is what it's trying to say.

> > LayoutTests/fast/filesystem/restricted-chars.html:14
> > +    ROOT.getFile('a\\b') raises 13
> 
> Also try getFile('a/b'), since there's no directory 'a'.

I think that's the test for getFile but not for restricted chars/names.

> > LayoutTests/fast/filesystem/restricted-names.html:58
> > +    file.copyTo(dir, 'foo.') raises 13
> 
> Test moveTo as well.
> 
> > LayoutTests/fast/filesystem/script-tests/parallel-operations.js:53
> > +    helper.run(function() { b.moveTo(fileSystem.root, 'b2', done, errorCallback); });
> 
> You're copying a to b, but you're also moving b.  Is that a race condition?
------- Comment #16 From 2010-10-01 16:50:11 PST -------
Oops wrongly hit 'commit' while writing the response... anyway.  (I wanted to send it out when I update the patch)

> You're copying a to b, but you're also moving b.  Is that a race condition?

This should be ok in our implementation (as they're executed in a serialized way), but could be problematic in other ports... well I'll fix it.
------- Comment #17 From 2010-10-01 16:58:22 PST -------
(From update of attachment 69306 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69306&action=review

>>> LayoutTests/fast/filesystem/op-get-entry.html:74
>>> +    # The entry's path must not be used if the given path is a full path.
>> 
>> I'm not sure what this comment means.
> 
> If the given path is a relative path we construct a new path by concatenating entry.fullPath, the given path and the new name.  Otherwise (== the given path is an absolute path) we'll just use the given path.
> 
> ... is what it's trying to say.

Ah, how about something like "Test that relative paths are relative to the DirectoryEntry to which they're supplied."?
------- Comment #18 From 2010-10-04 01:48:44 PST -------
Created an attachment (id=69606) [details]
Patch

Rewrote the tests.
------- Comment #19 From 2010-10-04 01:51:25 PST -------
Have rewritten in pure JS.

(In reply to comment #14)
> (From update of attachment 69306 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69306&action=review
> Add some relative path tests with "." and "..", e.g. "/parent/a/a/../../a/d", "/parent/a/./././../././a/d".
> Also make sure that we can't reach out of the sandbox by verifying that e.g. "/../../../../../../../a" == "/a".

Currently all of them throw INVALID_MODIFICATION_ERR since the implementation performs path/validity check before cleaning up those relative components.  I put FIXME comment in the test for now.

> > LayoutTests/fast/filesystem/script-tests/parallel-operations.js:53
> > +    helper.run(function() { b.moveTo(fileSystem.root, 'b2', done, errorCallback); });
> 
> You're copying a to b, but you're also moving b.  Is that a race condition?

Fixed... and this test is moved to another patch (with a different name: async-operations.js):
https://bugs.webkit.org/show_bug.cgi?id=47044
------- Comment #20 From 2010-10-06 11:37:15 PST -------
(From update of attachment 69606 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69606&action=review

> LayoutTests/fast/filesystem/resources/op-get-entry.js:51
> +            // FIXME: For now they throw an error because they fail the check for restricted-names: 'a path component should not end with period'.

Is there a bug logged for that?

> LayoutTests/fast/filesystem/resources/op-get-parent.js:10
> +            function(helper) { helper.getParent('/'); },

How do we verify the results of the getParent?

> LayoutTests/fast/filesystem/resources/op-read-directory.js:20
> +            function(helper) { helper.readDirectory('/'); }

How are the results of the readDirectory call specified and verified?  Is it just by comparison against the golden results, or is there some way to self-check?

> LayoutTests/fast/filesystem/resources/op-tests-helper.js:15
> +            return obj;

Perhaps obj + "" here and below?  Otherwise you haven't done the string conversion.

> LayoutTests/fast/filesystem/resources/op-tests-helper.js:161
> +                shouldBe.apply(this, ['this.environment[this.entry.fullPath].fullPath', '"' + entry.fullPath + '"']);

Could you point me to where this.entry gets set?

> LayoutTests/fast/filesystem/resources/op-tests-helper.js:252
> +        this.getMetadata = function(entry, expectedErrorCode)

I don't see a test for this.
------- Comment #21 From 2010-10-06 15:49:53 PST -------
(In reply to comment #20)
> (From update of attachment 69606 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69606&action=review
> 
> > LayoutTests/fast/filesystem/resources/op-get-entry.js:51
> > +            // FIXME: For now they throw an error because they fail the check for restricted-names: 'a path component should not end with period'.
> 
> Is there a bug logged for that?

Just filed - bug 47309.

> > LayoutTests/fast/filesystem/resources/op-get-parent.js:10
> > +            function(helper) { helper.getParent('/'); },
> 
> How do we verify the results of the getParent?

Currently they don't have explicit assertions.  At least the helper explicitly prints out the parent path so that we can match the actual results with expections (and it's not throwing an error).

> > LayoutTests/fast/filesystem/resources/op-read-directory.js:20
> > +            function(helper) { helper.readDirectory('/'); }
> 
> How are the results of the readDirectory call specified and verified?  Is it just by comparison against the golden results, or is there some way to self-check?

Ditto.  No self-check for now.

> > LayoutTests/fast/filesystem/resources/op-tests-helper.js:15
> > +            return obj;
> 
> Perhaps obj + "" here and below?  Otherwise you haven't done the string conversion.

Will fix.

> > LayoutTests/fast/filesystem/resources/op-tests-helper.js:161
> > +                shouldBe.apply(this, ['this.environment[this.entry.fullPath].fullPath', '"' + entry.fullPath + '"']);
> 
> Could you point me to where this.entry gets set?

Right before the line.
LayoutTests/fast/filesystem/resources/op-tests-helper.js:161
+                 this.entry = entry;

> > LayoutTests/fast/filesystem/resources/op-tests-helper.js:252
> > +        this.getMetadata = function(entry, expectedErrorCode)
> 
> I don't see a test for this.

For getMetadata I wasn't able to make self-assertive one very quickly (and the patch was already big so I stopped adding it).  I'll separately make it.  Filed a new bug 47311.
------- Comment #22 From 2010-10-06 16:50:13 PST -------
(From update of attachment 69606 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=69606&action=review

>>> LayoutTests/fast/filesystem/resources/op-tests-helper.js:161
>>> +                shouldBe.apply(this, ['this.environment[this.entry.fullPath].fullPath', '"' + entry.fullPath + '"']);
>> 
>> Could you point me to where this.entry gets set?
> 
> Right before the line.
> LayoutTests/fast/filesystem/resources/op-tests-helper.js:161
> +                 this.entry = entry;

Ha!  OK, thanks.
------- Comment #23 From 2010-10-06 23:24:38 PST -------
r=me 

Please address Eric's concern on landing:
> Perhaps obj + "" here and below?  Otherwise you haven't done the string conversion.

Will fix.
------- Comment #24 From 2010-10-07 14:10:28 PST -------
Committed r69339: <http://trac.webkit.org/changeset/69339>