| Summary: | Add layout tests for FileSystem API | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | dumi, ericu, fishd, levin, michaeln | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 42903 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Kinuko Yasuda
2010-08-30 18:33:31 PDT
Created attachment 67866 [details]
Patch
Comment on attachment 67866 [details] Patch 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? (In reply to comment #2) > (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. 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.) Created attachment 67986 [details]
Patch
Created attachment 67987 [details]
Patch
(In reply to comment #2) > (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. 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 on attachment 67987 [details] Patch 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 Created attachment 69245 [details]
Patch
(In reply to comment #7) > (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 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. Created attachment 69304 [details]
Patch
Rebased.
Created attachment 69306 [details]
Patch
Comment on attachment 69306 [details] Patch 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]. (In reply to comment #12) > (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. 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 on attachment 69306 [details] Patch 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? Great thanks. (In reply to comment #14) > (From update of attachment 69306 [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? 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 on attachment 69306 [details] Patch 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."? Created attachment 69606 [details]
Patch
Rewrote the tests.
Have rewritten in pure JS. (In reply to comment #14) > (From update of attachment 69306 [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 on attachment 69606 [details] Patch 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. (In reply to comment #20) > (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? 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 on attachment 69606 [details] Patch 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. r=me
Please address Eric's concern on landing:
> Perhaps obj + "" here and below? Otherwise you haven't done the string conversion.
Will fix.
Committed r69339: <http://trac.webkit.org/changeset/69339> |