Closed Bug 1234287 Opened 4 years ago Closed 4 years ago

Refactor webconsole netlogging tests

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: linclark, Assigned: linclark)

References

Details

Attachments

(1 file, 5 obsolete files)

The browser_webconsole_netlogging should probably be split into different tests. Additionally, both it and browser_console_netlogging (added in Bug #1064458) should be refactored to use add_task.
Depends on: 1064458
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attached patch Bug1234287.patch (obsolete) — Splinter Review
This is a WIP. It is on hold until people are back from the holidays.

This patch:
- Switches to add_task() in browser_console_netlogging
- Extracts the network panel test from browser_webconsole_netlogging, and switches it to add_task()
- Renames a file

Left to do:
- Switch the remainder of browser_webconsole_netlogging to use add_task()
- Possibly refactor the rewritten tests to remove the waitForRequest() function that I added.

I'm not sure whether the existing tests that are still in browser_webconsole_netlogging are actually checking what they should be checking. I will discuss this with bgrins when he's back.
Attached patch Bug1234287.patch (obsolete) — Splinter Review
This patch:

- Breaks out the panel test from browser_webconsole_netlogging into its own test
- Switches all tests to use add_task()

If you have any other suggestions for additional refactoring, I'm happy to  hear them.
Attachment #8702010 - Attachment is obsolete: true
Attachment #8705373 - Flags: review?(bgrinstead)
Comment on attachment 8705373 [details] [diff] [review]
Bug1234287.patch

Review of attachment 8705373 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review because of the comment about e10s / ContentTask but everything else is looking great

::: devtools/client/webconsole/test/browser.ini
@@ +2,5 @@
>  tags = devtools
>  subsuite = devtools
>  support-files =
>    head.js
> +  help_netlogging.js

Super nit: we usually name these files helper_*.  Also, the 3 functions in the helper file look like they could live in head.js.  Not necessarily against splitting them out into a helper but it might be easier for other tests to use them if we put them in the head.  What do you think?

::: devtools/client/webconsole/test/browser_webconsole_netlogging.js
@@ +79,5 @@
>  
> +add_task(function* testFormSubmission() {
> +  let hud = yield loadTestNetworkPage();
> +
> +  let form = content.document.querySelector("form");

This was previously using ContentTask.spawn which is more friendly for e10s than directly accessing `content`.  Is there a reason this won't work with the refactor?

  let finishedRequest = finishRequest();
  ContentTask.spawn(gBrowser.selectedBrowser, { }, function*() {
    content.document.querySelector("form").submit();
  });
  let request = yield finishedRequest;

@@ +103,5 @@
> +    "Response body's beginning is okay");
> +});
> +
> +function finishRequest() {
> +  let deferred = promise.defer();

Nit: We've been using DOM Promises instead of promise.defer whenever possible:

return new Promise(resolve => {
  HUDService.lastFinishedRequest.callback = request => {
    resolve(request)
  };
});

::: devtools/client/webconsole/test/help_netlogging.js
@@ +54,5 @@
> + * @param object hud
> + * @param object request
> + */
> +function getResponseContent(hud, request) {
> +  let deferred = promise.defer();

Please return new Promise

@@ +67,5 @@
> + *
> + * @param object hud
> + * @param object request
> + */
> +function getPostData(hud, request) {

Ditto
Attachment #8705373 - Flags: review?(bgrinstead)
Blocks: 1239029
Attached patch Bug1234287.patch (obsolete) — Splinter Review
Thanks for catching those things and for the suggestion.

The changes from the last patch are:
- Generalized the helper methods into loadPageAndGetHud() and getPacket() methods, which are now in head.js. Let me know if you think any of this should change.
- Added ContentTask.spawn back in
- Switched to DOM Promises
- Added a registerCleanupFunction to address Bug 1240070

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97d38a962286
Attachment #8705373 - Attachment is obsolete: true
Attachment #8709579 - Flags: review?(bgrinstead)
Blocks: 1240070
Comment on attachment 8709579 [details] [diff] [review]
Bug1234287.patch

Review of attachment 8709579 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  I'd like to take a closer look at browser_webconsole_netlogging.js so not clearing flag but interested in your thoughts about changing the sig of loadPageAndGetHud wrt the last finished request.

::: devtools/client/webconsole/test/browser_console_netlogging.js
@@ +11,5 @@
>    "http://example.com/browser/devtools/client/webconsole/test/" +
>    "test-network-request.html";
>  
> +registerCleanupFunction(function() {
> +  HUDService.lastFinishedRequest.callback = null;

Maybe this cleanup function should happen inside of head.js so it covers all tests - there's already a registerCleanupFunction there.  But it's probably another bug since we'd want to also remove that block form existing tests that are doing it now.

::: devtools/client/webconsole/test/browser_webconsole_netlogging_panel.js
@@ +16,5 @@
> +});
> +
> +add_task(function* () {
> +  let request;
> +  HUDService.lastFinishedRequest.callback = req => request = req;

It looks like this is a pattern that could be moved into loadPageAndGetHud since it's needed in all three places and is a bit awkward since we are dealing with callbacks.  So loadPageAndGetHud could return {hud, lastFinishedRequest} or similar which could be destructured from the callees.  Also that function could do the bit with registering the cleanup function so that could be de-duplicated.

::: devtools/client/webconsole/test/head.js
@@ +87,5 @@
> + *
> + * @param string uri
> + *   The URI of the page to load.
> + * @param string consoleType
> + *   The console type, either "browserConsole" or "webConsole"

Should note that it defaults to "webConsole"
Comment on attachment 8709579 [details] [diff] [review]
Bug1234287.patch

Review of attachment 8709579 [details] [diff] [review]:
-----------------------------------------------------------------

browser_webconsole_netlogging.js looks good.  Clearing review as per Comment 5
Attachment #8709579 - Flags: review?(bgrinstead)
Attached patch Bug1234287.patch (obsolete) — Splinter Review
I did have it that way for a bit and then pulled the request logic out. I did that because I needed to wait for the request when the XHR fires. So in browser_webconsole_netlogging, I pulled it out into finishRequest().

In this patch, I moved this to head and changed the other two files to use it. This has the benefit of giving me a good place to put the registerCleanupFunction().

I could also change the signature of loadUriAndGetHud() and then use finishRequest() inside that function to get the request. I feel like it might be too specific of a helper at that point. Do you think those two actions (getting the HUD and the request) make sense together, generally? If you think so, then I'll make that change.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4765eb4b39b8
Attachment #8709579 - Attachment is obsolete: true
Attachment #8710667 - Flags: review?(bgrinstead)
Attached patch Bug1234287.patch (obsolete) — Splinter Review
I realized I forgot to make the third change (the comment update).
Attachment #8710667 - Attachment is obsolete: true
Attachment #8710667 - Flags: review?(bgrinstead)
Attachment #8710730 - Flags: review?(bgrinstead)
Comment on attachment 8710730 [details] [diff] [review]
Bug1234287.patch

Review of attachment 8710730 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/test/head.js
@@ +93,5 @@
> + * @return object
> + *   The HUD associated with the console
> + */
> +function* loadPageAndGetHud(uri, consoleType) {
> +  let { browser } = yield loadTab("data:text/html;charset=utf-8,Loading tab for tests";);

There's a syntax error / typo here
(In reply to Lin Clark from comment #7)
> Created attachment 8710667 [details] [diff] [review]
> Bug1234287.patch
> 
> I did have it that way for a bit and then pulled the request logic out. I
> did that because I needed to wait for the request when the XHR fires. So in
> browser_webconsole_netlogging, I pulled it out into finishRequest().
> 
> In this patch, I moved this to head and changed the other two files to use
> it. This has the benefit of giving me a good place to put the
> registerCleanupFunction().

Cool, works for me.  Maybe we should rename this to waitForFinishedRequest() or something so it doesn't sound like it's causing a request to finish?
Comment on attachment 8710730 [details] [diff] [review]
Bug1234287.patch

Review of attachment 8710730 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great overall, please see Comments 9/10 and then r=me with a green try push
Attachment #8710730 - Flags: review?(bgrinstead) → review+
Attached patch Bug1234287.patchSplinter Review
Good call on the function name. I changed that (and fixed the syntax error :-X).

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfa4f15aacd&selectedJob=15763187
Attachment #8710730 - Attachment is obsolete: true
Attachment #8711216 - Flags: review?(bgrinstead)
Attachment #8711216 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/65e26c411007
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1242234
Depends on: 1242318
Depends on: 1243499
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.