32596 – Refactor Window object tests to use a shared list of property names

Bug 32596 - Refactor Window object tests to use a shared list of property names

Summary: Refactor Window object tests to use a shared list of property names

Status:	ASSIGNED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Tools / Tests (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Mac (Intel) OS X 10.5

Importance:	P2 Normal
Assignee:	Kent Hansen

URL:
Keywords:

Depends on:
Blocks:

Reported:	2009-12-15 22:31 PST by Kent Hansen
Modified:	2010-07-14 07:31 PDT (History)
CC List:	4 users (show)

See Also:

Attachments
Proposed patch (85.04 KB, patch) 2009-12-15 22:33 PST, Kent Hansen	no flags	Details \| Formatted Diff \| Diff
Updated patch (fix URL in ChangeLog) (85.04 KB, patch) 2009-12-15 22:37 PST, Kent Hansen	no flags	Details \| Formatted Diff \| Diff
Updated patch (fix spelling error in ChangeLog) (85.04 KB, patch) 2009-12-15 22:56 PST, Kent Hansen	commit-queue: commit-queue-	Details \| Formatted Diff \| Diff
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-	Details \| Formatted Diff \| Diff
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-	Details \| Formatted Diff \| Diff
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-	Details \| Formatted Diff \| Diff
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	Details \| Formatted Diff \| Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Kent Hansen 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.)

Comment 1 Kent Hansen 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.

Comment 2 WebKit Review Bot 2009-12-15 22:36:52 PST

style-queue ran check-webkit-style on attachment 44941 [details] without any errors.

Comment 3 Kent Hansen 2009-12-15 22:37:20 PST

Created attachment 44942 [details]
Updated patch (fix URL in ChangeLog)

Comment 4 WebKit Review Bot 2009-12-15 22:42:19 PST

style-queue ran check-webkit-style on attachment 44942 [details] without any errors.

Comment 5 Kent Hansen 2009-12-15 22:56:47 PST

Created attachment 44945 [details]
Updated patch (fix spelling error in ChangeLog)

Comment 6 WebKit Review Bot 2009-12-15 23:27:22 PST

style-queue ran check-webkit-style on attachment 44945 [details] without any errors.

Comment 7 Darin Adler 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.

Comment 8 WebKit Commit Bot 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

Comment 9 Eric Seidel (no email) 2009-12-16 17:45:16 PST

This looks like a related failure.

Comment 10 Kent Hansen 2009-12-17 05:01:54 PST

Created attachment 45055 [details]
Revised patch (use "var" keyword, update LayoutTests/platform/mac/objc-wrapper-identity)

Comment 11 WebKit Review Bot 2009-12-17 05:03:39 PST

style-queue ran check-webkit-style on attachment 45055 [details] without any errors.

Comment 12 Kent Hansen 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.

Comment 13 Kent Hansen 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.

Comment 14 Darin Adler 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

Comment 15 Kent Hansen 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)

Comment 16 WebKit Review Bot 2009-12-21 08:31:01 PST

style-queue ran check-webkit-style on attachment 45331 [details] without any errors.

Comment 17 WebKit Commit Bot 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

Comment 18 Kent Hansen 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.

Comment 19 Kent Hansen 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)

Comment 20 WebKit Review Bot 2009-12-21 12:06:35 PST

style-queue ran check-webkit-style on attachment 45341 [details] without any errors.

Comment 21 WebKit Commit Bot 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

Comment 22 Kent Hansen 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.

Comment 23 Eric Seidel (no email) 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. :(

Comment 24 Kent Hansen 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.

Comment 25 Kent Hansen 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?

Comment 26 Kent Hansen 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?

Comment 27 Eric Seidel (no email) 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.

Comment 28 Eric Seidel (no email) 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!

Comment 29 Eric Seidel (no email) 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.

Comment 30 Eric Seidel (no email) 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.

Comment 31 Eric Seidel (no email) 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.

Comment 32 Kent Hansen 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!