Bug 147787 - Web Inspector: use different namespaces in test fixtures for protocol tests and frontend tests
Summary: Web Inspector: use different namespaces in test fixtures for protocol tests a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-07 11:40 PDT by BJ Burg
Modified: 2015-08-11 11:28 PDT (History)
10 users (show)

See Also:


Attachments
Proposed Fix (243.72 KB, patch)
2015-08-10 16:01 PDT, BJ Burg
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (674.43 KB, application/zip)
2015-08-10 16:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (662.47 KB, application/zip)
2015-08-10 16:18 PDT, Build Bot
no flags Details
Proposed Fix (247.53 KB, patch)
2015-08-10 16:35 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-08-07 11:40:13 PDT
IOW, rename InspectorTest to ProtocolTest or something similar in protocol-test.js.
Comment 1 BJ Burg 2015-08-07 23:02:50 PDT
Current refactoring:

* Protocol-related things in ProtocolTestStub get renamed to InspectorProtocol
* Test things (log, assert, etc) get renamed to ProtocolTest
* Everything else is left alone

In the next refactor, Test things will be refactored to be shared between Protocol and Frontend tests. Until then, the two implementations have different names.
Comment 2 BJ Burg 2015-08-10 16:01:08 PDT
Created attachment 258664 [details]
Proposed Fix
Comment 3 Build Bot 2015-08-10 16:15:04 PDT
Comment on attachment 258664 [details]
Proposed Fix

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

New failing tests:
http/tests/inspector/console/access-inspected-object.html
http/tests/inspector/page/loading-iframe-document-node.html
Comment 4 Build Bot 2015-08-10 16:15:06 PDT
Created attachment 258667 [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 5 Build Bot 2015-08-10 16:18:45 PDT
Comment on attachment 258664 [details]
Proposed Fix

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

New failing tests:
http/tests/inspector/console/access-inspected-object.html
http/tests/inspector/page/loading-iframe-document-node.html
Comment 6 Build Bot 2015-08-10 16:18:47 PDT
Created attachment 258669 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Devin Rousso 2015-08-10 16:32:11 PDT
Comment on attachment 258664 [details]
Proposed Fix

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

> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:113
> +InspectorProtocol.checkForError = function(responseObject)

Will this also work for functions that return promises?  Also, I'm not sure about whether this is true or not, but are there any cases where a callback function/promise won't have a responseObject?  If so, I think you should check to see that the response object exists (and if it doesn't then throw the same error).  If that isn't true, you can ignore me.

That's really the only comment I have.
Comment 8 BJ Burg 2015-08-10 16:35:49 PDT
Created attachment 258673 [details]
Proposed Fix

I missed the few http/tests/inspector/ tests.
Comment 9 BJ Burg 2015-08-11 06:07:35 PDT
Comment on attachment 258664 [details]
Proposed Fix

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

>> LayoutTests/http/tests/inspector/resources/ProtocolTestStub.js:113
>> +InspectorProtocol.checkForError = function(responseObject)
> 
> Will this also work for functions that return promises?  Also, I'm not sure about whether this is true or not, but are there any cases where a callback function/promise won't have a responseObject?  If so, I think you should check to see that the response object exists (and if it doesn't then throw the same error).  If that isn't true, you can ignore me.
> 
> That's really the only comment I have.

This is reordered code. I want to delete it eventually (the protocol layer should handle this, not test code), but not in this patch.
Comment 10 WebKit Commit Bot 2015-08-11 11:28:52 PDT
Comment on attachment 258673 [details]
Proposed Fix

Clearing flags on attachment: 258673

Committed r188267: <http://trac.webkit.org/changeset/188267>
Comment 11 WebKit Commit Bot 2015-08-11 11:28:56 PDT
All reviewed patches have been landed.  Closing bug.