Bug 148740 – Web Inspector: Expand Console domain test coverage

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Expand test coverage for all the (used) Console commands and events. Things like:

    [command] Console.addInspectedNode
    [command] Console.clearMessages
    [event] Console.messagesCleared
    [event] Console.messageRepeatCountUpdated

[Joseph Pecoraro]

Created attachment 260496 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 260496 [details]
[PATCH] Proposed Fix

Attachment 260496 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/137098

New failing tests:
inspector/console/messagesCleared.html

[Build Bot]

Created attachment 260504 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Blaze Burg]

Comment on attachment 260496 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260496&action=review

These are overall great tests, and really good areas to get more coverage (based on how many regressions lately..)

One thing I think we should try to do more is to make test cases not depend on each others' execution. This antipattern (test dependence) is a classic source of test flakiness, especially when modifying/extending existing tests and unintentionally breaking subtle invariants. I think this is easy to achieve for these tests, usually by explicitly clearing messages as the test's 'setup' action. messageRepeatCountUpdated.html looks like the only test with overly-dependent testcases.

> LayoutTests/inspector/console/addInspectedNode.html:27
> +                InspectorTest.expectThat(remoteObject.isUndefined(), "$0 should be undefined.");

Do we have a way to clear $0 programmatically? If so, we should do that as each testcase's `setup` action to make these not dependent.

> LayoutTests/inspector/console/addInspectedNode.html:49
> +        name: "SetBadValue",

SetInvalidInspectedNode

> LayoutTests/inspector/console/addInspectedNode.html:50
> +        description: "Set an invalid inspected node id should error an $0 should still be Node A.",

"If an invalid inspected node is set, it shouldn't be bound to $0."

> LayoutTests/inspector/console/addInspectedNode.html:81
> +    // FIXME: What should $0 be after reloading / navigating the page. Should it be cleared?

I think it should be reset to undefined, otherwise there might be a cross-origin reference.

> LayoutTests/inspector/console/addInspectedNode.html:84
> +        WebInspector.domTreeManager.querySelector(documentNode.id, "#a", (contentNodeId) => {

I know it's tedious, but maybe this should check if there's an error and die. It will make tests fail instead of time out if domTreeManager's methods get broken somehow.

> Source/JavaScriptCore/inspector/protocol/Console.json:66
> +            "description": "Enables console to refer to the node with given id via $0 (see Command Line API for more details)."

Might also add that invalid ids are ignored.

[Radar WebKit Bug Importer]

<rdar://problem/22569880>

[Joseph Pecoraro]

Comment on attachment 260496 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260496&action=review

>> LayoutTests/inspector/console/addInspectedNode.html:27
>> +                InspectorTest.expectThat(remoteObject.isUndefined(), "$0 should be undefined.");
> 
> Do we have a way to clear $0 programmatically? If so, we should do that as each testcase's `setup` action to make these not dependent.

We do not yet. I'm thinking we should do that on reload/navigation.

>> LayoutTests/inspector/console/addInspectedNode.html:49
>> +        name: "SetBadValue",
> 
> SetInvalidInspectedNode

Okay! I did name this poorly. That said, I've been pretty consistent about "BadFoo" instead of "InvalidFoo". Should I move to Invalid?

>> LayoutTests/inspector/console/addInspectedNode.html:50
>> +        description: "Set an invalid inspected node id should error an $0 should still be Node A.",
> 
> "If an invalid inspected node is set, it shouldn't be bound to $0."

Okay! I've been pretty consistent about "shout not" instead of "shouldn't". I hate apostrophes.

> LayoutTests/inspector/console/messagesCleared.html:68
> +        name: "ClearViaPageNavigation",
> +        description: "Navigating the page should trigger Console.messagesCleared.",
> +        test: (resolve, reject) => {
> +            InspectorTest.evaluateInPage("window.location.href = window.location.href + '?navigate-for-test'");

This is a flakey way to test. Just testing reload should be enough for this scenario anyways, and makes this test pass consistently for me.

[Joseph Pecoraro]

Created attachment 260569 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

> Okay! I've been pretty consistent about "shout not" instead of "shouldn't".

Err, "should not", heh.

[Blaze Burg]

Comment on attachment 260569 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260569&action=review

r=me

> LayoutTests/inspector/console/addInspectedNode.html:81
> +    // FIXME: What should $0 be after reloading / navigating the page. Should it be cleared?

Ordinarily, it should be reset to undefined/null, but the inspector state cookie may re-select a DOM element on reload.

> LayoutTests/inspector/console/messageRepeatCountUpdated.html:175
> +                    WebInspector.logManager.removeEventListener(WebInspector.LogManager.Event.PreviousMessageRepeatCountUpdated, repeatListener, null);

You could use removeAllListeners() instead.

[Joseph Pecoraro]

(In reply to comment #9)
> Comment on attachment 260569 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260569&action=review
> 
> r=me
> 
> > LayoutTests/inspector/console/addInspectedNode.html:81
> > +    // FIXME: What should $0 be after reloading / navigating the page. Should it be cleared?
> 
> Ordinarily, it should be reset to undefined/null, but the inspector state
> cookie may re-select a DOM element on reload.
> 
> > LayoutTests/inspector/console/messageRepeatCountUpdated.html:175
> > +                    WebInspector.logManager.removeEventListener(WebInspector.LogManager.Event.PreviousMessageRepeatCountUpdated, repeatListener, null);
> 
> You could use removeAllListeners() instead.

I feel like this function is dangerous. It might remove a listener you don't know about and shouldn't have removed. If anyone should use it, it should be some object removing all listeners on itself. Fortunately our code base doesn't use it at all. I've toyed with the idea of just removing it.

[WebKit Commit Bot]

Comment on attachment 260569 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 260569

Committed r189373: <http://trac.webkit.org/changeset/189373>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.