| Summary: | Refactor Window object tests to use a shared list of property names | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Hansen <kent.hansen> | ||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Kent Hansen <kent.hansen> | ||||||||||||||||||
| Status: | ASSIGNED --- | ||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, eric, hausmann, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Mac (Intel) | ||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Kent Hansen
2009-12-15 22:31:10 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.
style-queue ran check-webkit-style on attachment 44941 [details] without any errors.
Created attachment 44942 [details]
Updated patch (fix URL in ChangeLog)
style-queue ran check-webkit-style on attachment 44942 [details] without any errors.
Created attachment 44945 [details]
Updated patch (fix spelling error in ChangeLog)
style-queue ran check-webkit-style on attachment 44945 [details] without any errors.
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 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 This looks like a related failure. Created attachment 45055 [details]
Revised patch (use "var" keyword, update LayoutTests/platform/mac/objc-wrapper-identity)
style-queue ran check-webkit-style on attachment 45055 [details] without any errors.
(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. (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 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
Created attachment 45331 [details]
Revised patch (don't modify auto-generated wrappers, add comment to window-properties test)
style-queue ran check-webkit-style on attachment 45331 [details] without any errors.
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 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. Created attachment 45341 [details]
Revised patch (add back script tags to non-auto-generated wrappers to fix path issue)
style-queue ran check-webkit-style on attachment 45341 [details] without any errors.
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 (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. Created attachment 45342 [details]
failure diff from commit-queue machine
Sorry. the commit-queue needs to be taught to upload failure results. :(
(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. 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? 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? (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 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 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 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 on attachment 44945 [details]
Updated patch (fix spelling error in ChangeLog)
Clearing Darin's r+ on this obsolete patch.
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! |