* 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
Created attachment 260496 [details] [PATCH] Proposed Fix
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
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
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.
<rdar://problem/22569880>
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.
Created attachment 260569 [details] [PATCH] Proposed Fix
> Okay! I've been pretty consistent about "shout not" instead of "shouldn't". Err, "should not", heh.
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.
(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.
Comment on attachment 260569 [details] [PATCH] Proposed Fix Clearing flags on attachment: 260569 Committed r189373: <http://trac.webkit.org/changeset/189373>
All reviewed patches have been landed. Closing bug.