ASSIGNED 32596
Refactor Window object tests to use a shared list of property names 
https://bugs.webkit.org/show_bug.cgi?id=32596
Summary Refactor Window object tests to use a shared list of property names
Kent Hansen
Reported 2009-12-15 22:31:10 PST
Some of the LayoutTests use for-in statements on the Window object to create a list of property names to use for testing. This approach is not very robust. The issues can be summed up as follows: 1) Adding new properties on the Window object breaks existing tests. It's especially unfortunate when commits like r51997 are necessary just to keep existing tests working. If the test inputs were decoupled from the Window object itself, such situations would be avoided. (The script for LayoutTests/fast/dom/wrapper-identity, a test that _does_ use an a priori list of property names, begins with the comment "We use a static list of window properties to avoid breaking when new properties are added.") 2) The for-in statement only reports enumerable properties. The Window object has several non-enumerable properties that are relevant for testing; it's a shame that these are excluded. On a related note, it's not possible to make an existing enumerable property non-enumerable without breaking tests (this is a blocker for https://bugs.webkit.org/show_bug.cgi?id=28771). 3) The for-in statement dredges up properties from the prototype chain. In order to ensure that only properties on the Window object itself are tested, the loop body should therefore call window.hasOwnProperty(). But none of the tests perform this check, which means that they can erroneously process inherited (i.e. irrelevant) properties. (The tests for the "inner.isInner" property in LayoutTests/fast/dom/prototype-inheritance-expected.txt is the only place I've found that demonstrates this problem.)
Attachments
Proposed patch (85.04 KB, patch)
2009-12-15 22:33 PST, Kent Hansen 		no flags
DetailsFormatted DiffDiff
Updated patch (fix URL in ChangeLog) (85.04 KB, patch)
2009-12-15 22:37 PST, Kent Hansen 		no flags
DetailsFormatted DiffDiff
Updated patch (fix spelling error in ChangeLog) (85.04 KB, patch)
2009-12-15 22:56 PST, Kent Hansen 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
Revised patch (use "var" keyword, update LayoutTests/platform/mac/objc-wrapper-identity) (112.00 KB, patch)
2009-12-17 05:01 PST, Kent Hansen 		darin: review-
darin: commit-queue-
DetailsFormatted DiffDiff
Revised patch (don't modify auto-generated wrappers, add comment to window-properties test) (113.35 KB, patch)
2009-12-21 08:30 PST, Kent Hansen 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
Revised patch (add back script tags to non-auto-generated wrappers to fix path issue) (114.60 KB, patch)
2009-12-21 12:05 PST, Kent Hansen 		eric: review-
commit-queue: commit-queue-
DetailsFormatted DiffDiff
failure diff from commit-queue machine (4.73 KB, text/plain)
2009-12-21 12:55 PST, Eric Seidel (no email) 		no flags
Details
Draft of patch for new approach (dynamic+static list) (104.02 KB, patch)
2009-12-30 04:15 PST, Kent Hansen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Kent Hansen
Comment 1 2009-12-15 22:33:14 PST
Created attachment 44941 [details] Proposed patch Moved the staticWindowProperties array to its own file and updated relevant tests to use that array. Reordered the list (and results) so it's sorted again. Added a bunch of properties that were missing.
WebKit Review Bot
Comment 2 2009-12-15 22:36:52 PST
style-queue ran check-webkit-style on attachment 44941 [details] without any errors.
Kent Hansen
Comment 3 2009-12-15 22:37:20 PST
Created attachment 44942 [details] Updated patch (fix URL in ChangeLog)
WebKit Review Bot
Comment 4 2009-12-15 22:42:19 PST
style-queue ran check-webkit-style on attachment 44942 [details] without any errors.
Kent Hansen
Comment 5 2009-12-15 22:56:47 PST
Created attachment 44945 [details] Updated patch (fix spelling error in ChangeLog)
WebKit Review Bot
Comment 6 2009-12-15 23:27:22 PST
style-queue ran check-webkit-style on attachment 44945 [details] without any errors.
Darin Adler
Comment 7 2009-12-16 09:38:12 PST
Comment on attachment 44945 [details] Updated patch (fix spelling error in ChangeLog) What this removes is any test that checks that no extra properties on the Window object are present. I would like to see us add one test for that. It's good not to have multiple tests for it, but I think it's good to have one to help remind people to keep the list of window properties up to date. That test can have whatever platform exceptions we need coded into it. > -FAIL inner.isInner.isInner should be true. Was false. > -FAIL inner.isInner.constructor.isInner should be true. Was false. Is it a good thing to skip testing this? Should we add something back so this case is tested? > +staticWindowProperties = [ I think this should say "var staticWindowProperties" or "const staticWindowProperties" for better efficiency. > diff --git a/LayoutTests/fast/js/global-constructors.html b/LayoutTests/fast/js/global-constructors.html > index d08d6f5..684a9ed 100644 > --- a/LayoutTests/fast/js/global-constructors.html > +++ b/LayoutTests/fast/js/global-constructors.html > @@ -7,6 +7,7 @@ > <body> > <p id="description"></p> > <div id="console"></div> > +<script src="../dom/resources/static-window-properties.js"></script> > <script src="script-tests/global-constructors.js"></script> > <script src="resources/js-test-post.js"></script> > </body> Changing the test in this fashion means it should move into the fast/dom directory. The old test could run in a non-browser context, so it made sense to have it in fast/js, although we'd need different test results for that case. I'm going to say r=me on this as-is, but ideally I'd like to see some refinement and followup in future patches.
WebKit Commit Bot
Comment 8 2009-12-16 09:48:31 PST
Comment on attachment 44945 [details] Updated patch (fix spelling error in ChangeLog) Rejecting patch 44945 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11777 test cases. platform/mac/fast/dom/objc-wrapper-identity.html -> failed Exiting early after 1 failures. 9617 tests run. 291.31s total testing time 9616 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/128629
Eric Seidel (no email)
Comment 9 2009-12-16 17:45:16 PST
This looks like a related failure.
Kent Hansen
Comment 10 2009-12-17 05:01:54 PST
Created attachment 45055 [details] Revised patch (use "var" keyword, update LayoutTests/platform/mac/objc-wrapper-identity)
WebKit Review Bot
Comment 11 2009-12-17 05:03:39 PST
style-queue ran check-webkit-style on attachment 45055 [details] without any errors.
Kent Hansen
Comment 12 2009-12-17 05:31:01 PST
(In reply to comment #7) > (From update of attachment 44945 [details]) > What this removes is any test that checks that no extra properties on the > Window object are present. I would like to see us add one test for that. It's > good not to have multiple tests for it, but I think it's good to have one to > help remind people to keep the list of window properties up to date. That test > can have whatever platform exceptions we need coded into it. Well, fast/dom/Window/get-set-properties will fail when new properties are added, so people will still need to update that test. But you're right, there's no hint that you perhaps need to update the shared list of properties too, which means the list can rot (which is what's happened already, since I had to add a ton of missing properties to make this patch work). Like Eric suggested in https://bugs.webkit.org/show_bug.cgi?id=28771, ideally the list should be auto-generated (from DOMWindow.idl?). > > > -FAIL inner.isInner.isInner should be true. Was false. > > -FAIL inner.isInner.constructor.isInner should be true. Was false. > > Is it a good thing to skip testing this? Should we add something back so this > case is tested? As I understand it, the purpose of the prototype-inheritance test is to test properties of the Window object itself, not properties inherited from Object.prototype (the subject of the bug for which the test was added is "Some constructor objects exposed on Window have the wrong prototype chain"). The isInner property can of course be injected into the property list to keep the above expected test output, but I'd prefer making a separate test if it's important. Maybe Eric can comment.
Kent Hansen
Comment 13 2009-12-17 05:33:40 PST
(In reply to comment #12) > Well, fast/dom/Window/get-set-properties will fail when new properties are > added, so people will still need to update that test. Oops, I meant fast/dom/Window/window-properties.
Darin Adler
Comment 14 2009-12-19 11:27:19 PST
Comment on attachment 45055 [details] Revised patch (use "var" keyword, update LayoutTests/platform/mac/objc-wrapper-identity) There's a problem with adding these "<script>" elements into automatically generated test wrappers such as wrapper-identity.html. The next time someone runs make-script-test-wrappers they will be overwritten. It would be better to add JavaScript code to load the script element into the script-tests files. Or you could add all of these into the exception list in the make-script-test-wrappers perl script -- that seems like an ugly solution. I'm not sure that automatically generating static-window-properties.js would be a big win. Many test results still need to be updated any time the list changes, so generating that file automatically would just reduce the number of files to edit by one. I think you should change fast/dom/Window/window-properties so that it there is some text in the test suggesting an update of static-window-properties.js. Someone might overlook that comment, but having the comment there gives them a fighting chance. And the comment should be in the output too so it's in the expected file as well. review- because this breaks make-script-test-wrappers -- this otherwise seems ready to go
Kent Hansen
Comment 15 2009-12-21 08:30:36 PST
Created attachment 45331 [details] Revised patch (don't modify auto-generated wrappers, add comment to window-properties test)
WebKit Review Bot
Comment 16 2009-12-21 08:31:01 PST
style-queue ran check-webkit-style on attachment 45331 [details] without any errors.
WebKit Commit Bot
Comment 17 2009-12-21 10:51:09 PST
Comment on attachment 45331 [details] Revised patch (don't modify auto-generated wrappers, add comment to window-properties test) Rejecting patch 45331 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11809 test cases. platform/mac/fast/dom/objc-wrapper-identity.html -> failed Exiting early after 1 failures. 9642 tests run. 296.21s total testing time 9641 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/137653
Kent Hansen
Comment 18 2009-12-21 11:45:45 PST
Ugh, the objc-wrapper-identity test doesn't successfully load static-window-properties.js because of path issues (the script tag has src="../../../../fast/dom/resources/wrapper-identity-base.js", but then wrapper-identity-base.js uses the relative path "resources/static-window-properties.js", which is wrong in this test...) I'm adding back the script tags to the wrapper-identity html files, since they are not auto-generated anyway.
Kent Hansen
Comment 19 2009-12-21 12:05:51 PST
Created attachment 45341 [details] Revised patch (add back script tags to non-auto-generated wrappers to fix path issue)
WebKit Review Bot
Comment 20 2009-12-21 12:06:35 PST
style-queue ran check-webkit-style on attachment 45341 [details] without any errors.
WebKit Commit Bot
Comment 21 2009-12-21 12:27:02 PST
Comment on attachment 45341 [details] Revised patch (add back script tags to non-auto-generated wrappers to fix path issue) Rejecting patch 45341 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11810 test cases. platform/mac/fast/dom/objc-wrapper-identity.html -> failed Exiting early after 1 failures. 9643 tests run. 296.72s total testing time 9642 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/136943
Kent Hansen
Comment 22 2009-12-21 12:36:07 PST
(In reply to comment #21) > platform/mac/fast/dom/objc-wrapper-identity.html -> failed Well, now I don't know why it's failing. It's failing locally with errors like "ReferenceError: Can't find variable: objCController", but it used to do that before too (I'm assuming the ObjectiveC bindings are not configured correctly on my end). Could somebody share the diff? Thanks.
Eric Seidel (no email)
Comment 23 2009-12-21 12:55:48 PST
Created attachment 45342 [details] failure diff from commit-queue machine Sorry. the commit-queue needs to be taught to upload failure results. :(
Kent Hansen
Comment 24 2009-12-21 13:32:33 PST
(In reply to comment #23) > Created an attachment (id=45342) [details] > failure diff from commit-queue machine > > Sorry. the commit-queue needs to be taught to upload failure results. :( Thanks! Yes, that would be great. On a related note, it would also be great if there were a way to add a patch to the queue "for trial purposes only"; just like the bot checks style, the commit bot could check if the patch applies cleanly & all tests pass, and automatically report failure results and turn off the review request if not (no need for a human to process it only for the commit bot to subsequently reject it anyway). I try to run all tests locally, but currently there are 100s of tests failing for me locally (unrelated to this particular task) and I can never be sure if my environment coincides with the commit bot's.
Kent Hansen
Comment 25 2009-12-21 13:38:29 PST
It looks like the ObjCController identityIsEqual() method doesn't handle number wrappers, only string wrappers, hence comparing a number wrapper to another wrapper that holds the same number returns false. Is this something that should be fixed in identityIsEqual() or should I change the expected test result to be false for all number properties?
Kent Hansen
Comment 26 2009-12-21 14:05:42 PST
Fix would be like r28101, but for Number. I don't speak Objective C, but it's something like if ([a isKindOfClass:[NSNumber class]] && [b isKindOfClass:[NSNumber class]]) return [(NSNumber *)a doubleValue] == [(NSNumber *)b doubleValue]; (In reply to comment #25) > It looks like the ObjCController identityIsEqual() method doesn't handle number > wrappers, only string wrappers, hence comparing a number wrapper to another > wrapper that holds the same number returns false. > Is this something that should be fixed in identityIsEqual() or should I change > the expected test result to be false for all number properties?
Eric Seidel (no email)
Comment 27 2009-12-22 17:40:09 PST
(In reply to comment #26) > Fix would be like r28101, but for Number. > I don't speak Objective C, but it's something like > > if ([a isKindOfClass:[NSNumber class]] && [b isKindOfClass:[NSNumber class]]) > return [(NSNumber *)a doubleValue] == [(NSNumber *)b doubleValue]; > > (In reply to comment #25) > > It looks like the ObjCController identityIsEqual() method doesn't handle number > > wrappers, only string wrappers, hence comparing a number wrapper to another > > wrapper that holds the same number returns false. > > Is this something that should be fixed in identityIsEqual() or should I change > > the expected test result to be false for all number properties? Seems we should just re-write this code as: return a == b || [a isEqual:b]; http://developer.apple.com/mac/library/documentation/cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html#//apple_ref/occ/intfm/NSObject/isEqual: I doubt the == would even be necessary.
Eric Seidel (no email)
Comment 28 2009-12-22 18:25:29 PST
Comment on attachment 45341 [details] Revised patch (add back script tags to non-auto-generated wrappers to fix path issue) Kent and I talked online. I expressed my preference for moving to a solution where we use a combination of a dynamic and static list. In order to avoid changing results, we should have the function take a window object and return a list, with the additional known hidden properties. In general I think this is a great piece of functionality to add to our testing framework!
Eric Seidel (no email)
Comment 29 2009-12-28 22:21:18 PST
Comment on attachment 45341 [details] Revised patch (add back script tags to non-auto-generated wrappers to fix path issue) Marking r- because this causes a layout test failure.
Eric Seidel (no email)
Comment 30 2009-12-28 22:21:36 PST
Comment on attachment 45331 [details] Revised patch (don't modify auto-generated wrappers, add comment to window-properties test) Clearing Darin's r+ on this obsolete patch.
Eric Seidel (no email)
Comment 31 2009-12-28 22:21:54 PST
Comment on attachment 44945 [details] Updated patch (fix spelling error in ChangeLog) Clearing Darin's r+ on this obsolete patch.
Kent Hansen
Comment 32 2009-12-30 04:15:18 PST
Created attachment 45654 [details] Draft of patch for new approach (dynamic+static list) I've intentionally not set the review? flag or included a detailed ChangeLog, because I'd like a discussion on the following points first (sorry it's long, but darn, this isn't so trivial :) ). I've ditched the previous approach of keeping a single static list of properties, and instead went for a dynamic+static approach as discussed with Eric. This patch causes more properties to be tested than before. This is due to two things: 1) Getting rid of the out-of-date static list of properties in wrapper-identity-base.js and replacing it with a dynamic approach; and 2) Testing of non-enumerable ECMA properties, by keeping a static list of them (this is the best we can do until JSC implements Object.getOwnPropertyNames). Testing more is good, I think. The only potential issue is that some of the expected test results added to wrapper-identity-expected.txt are for properties guarded in DOMWindow.idl (e.g. SVG). But since these are already part of window-properties-expected.txt, I'm going to assume we expect that these features are enabled on all platforms where the tests are run. A consequence of getting rid of the static list of properties in wrapper-identity-base.js is that the wrapper-identity tests _will_ break when a new window property is added. Based on feedback from Darin and Eric, I think this is what we want. Forgetting/not knowing about the static list is no longer possible (well, unless you're adding a non-enumerable property...). No need for warnings in the test output in hope of grabbing attention (like in the previous patch). The propertiesOnGlobalObject() function builds the list of window properties. I've put it in js-test-pre.js, which is included by most fast/js tests and many fast/dom tests. This is convenient because tests can readily use the function. However, it does cause js-test-pre.js to grow from ~260 lines to ~350 lines. I haven't been able to reliably measure whether this affects how long it takes to run the tests (crude timing seems to indicate it's about the same as before when running _all_ fast/js tests). I have no problems with moving it to a separate file, but then I'd have to reintroduce the evalFile() function in js-test-pre.js (see previous patch) so that I can load it even from tests that have automatically generated HTML wrappers. The duplication of "Ignore WebGL constructors" irks me a bit (note that it was already duplicated several times before this patch). At first I added those properties to the skip-set in propertiesOnGlobalObject(), but that felt wrong, because a test that actually _wants_ those properties to be returned wouldn't get them (but is there a use case for that? Don't know). It also felt wrong to put WebCore-specific property names in a file under fast/js. (On that note, as Darin pointed out in comment #7, though, fast/js/global-constructors should really be moved to fast/dom.) A way of solving both the "ignore WebGL" and "don't ignore anything" issue would be to make propertiesOnGlobalObject() even more generic, and have some helper function(s) (propertiesOnWindowObject() under fast/dom?) that parameterize it appropriately. I haven't changed the fast/dom/Window/window-properties test because that one is recursive. It could potentially be changed to use propertiesOnGlobalObject() for the top-level case, but I've left it out of this patch. I haven't changed fast/dom/Window/get-set-properties. That one also uses its own static lists of properties, divided into categories (read/write, read-only, string properties, functions). It might be possible to refactor it to use propertiesOnGlobalObject() in combination with the new ES5 function Object.getOwnPropertyDescriptor(), but I'd rather tackle that in a separate patch. I did not fix the spelling error ("properites") in prototype-inheritance.js, since that's an unrelated issue. :-P Finally, with this patch in place I'm able to fix the tests for https://bugs.webkit.org/show_bug.cgi?id=28771: 1) add the *Error properties to the notEnumerated array in propertiesOnGlobalObject(); and 2) remove the expected *Error test data from window-properties-expected. Whether 2) is correct or not depends on the interpretation of the test description ("properties that are reachable"; does !enumerable === !reachable? Apparently, since non-enumerable properties aren't already part of the test). In general, any test that uses for..in on the window directly will not be testing the ECMA *Error properties anymore, though (but I'm not aware of anyone else except window-properties). Mmm, Object.getOwnPropertyNames(), how I long for thee... Feedback on any of these concerns would be greatly appreciated!
Note You need to log in before you can comment on or make changes to this bug.