Closed Bug 1264626 Opened 4 years ago Closed 4 years ago

Write a test against the browser toolbox debugger

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 3 obsolete files)

I started writing a test in bug 1263629, but it is challenging to get it green on try and don't want to block this bug on getting this test green.
Attached patch patch v1 (obsolete) — Splinter Review
Priority: -- → P2
Assignee: nobody → poirot.alex
Attached patch patch v2 (obsolete) — Splinter Review
Rebased. Looks like this test is now green on try?!

This test check that overall breakpoint works in the browser debugger,
but also that we don't step into promise world.
This test used to fail without my recent promise patch.
Attachment #8742350 - Flags: review?(jryans)
Attachment #8741350 - Attachment is obsolete: true
Comment on attachment 8742350 [details] [diff] [review]
patch v2

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

Well, it seems reasonable to me, but I haven't reviewed many debugger tests.  I would ask r? from :ejpbruel or :jlong.

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +15,5 @@
> +      ["devtools.chrome.enabled", true],
> +      // Test-only pref to allow passing `testScript` argument to the browser
> +      // toolbox
> +      ["devtools.browser-toolbox.allow-unsafe-script", true],
> +      // On debug test slave, it takes more than the default time (20s)

Nit: Let's say "debug machines" or "debug test runner" or something instead of "slave"

@@ +36,5 @@
> +
> +  // Execute the function every second in order to trigger the breakpoint
> +  let interval = setInterval(s.plop, 1000);
> +
> +  // Be careful, this JS function is going to be executed in the addon toolbox,

Nit: browser toolbox

@@ +40,5 @@
> +  // Be careful, this JS function is going to be executed in the addon toolbox,
> +  // which lives in another process. So do not try to use any scope variable!
> +  let env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);
> +  let testScript = function () {
> +    dump("Opening the browser toolbox and debugger panel\n");

Can you use Task.jsm here?  I know it's going to the other process, but it would be nice if it's possible, so that it uses a format like other modern tests.
Attachment #8742350 - Flags: review?(jryans) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
James, this test is special as it targets the browser toolbox.
It craft the "testScript" function which is stringified and passed to the
browser toolbox process. It means that we can't use any head helper,
not any test harness function. We just execute code.
That may explain why this test is quite simple and may be more high level than regular one.

In parallel, I'm asking jryans to review all this special magic.
Attachment #8743204 - Flags: review?(jryans)
Attachment #8743204 - Flags: review?(jlong)
Attachment #8742350 - Attachment is obsolete: true
Comment on attachment 8743204 [details] [diff] [review]
patch v3

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

Does it make sense for such a test to be in the debugger folder, since it's mostly testing that tool?  It seems like more of test is debugger specific than about the browser toolbox generally.

General approach looks good!

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +38,5 @@
> +  let interval = setInterval(s.plop, 1000);
> +
> +  // Be careful, this JS function is going to be executed in the browser toolbox,
> +  // which lives in another process. So do not try to use any scope variable!
> +  let env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);

Nit: Wrap if more than 80 char

@@ +101,5 @@
> +      toolbox.destroy();
> +
> +    }).catch(error => {
> +      dump("Error while running code in the browser toolbox process:\n");
> +      dump(error+"\n");

Nit: error + "
Attachment #8743204 - Flags: review?(jryans) → review+
Comment on attachment 8743204 [details] [diff] [review]
patch v3

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

(sorry, was off most of last week)

What changed to allow this? I thought there was a technical limitation that make testing the browser toolbox hard.

I know jryans suggested moving this into the debugger, but I don't think we should do that, and it's for a stupid reason: the debugger tests are already too slow. You will most likely hit random timeouts on try if you move it to the debugger. We've already have to disable debugger tests on linux32 debug because they kept timing out, for no reason other than just taking too long. It sucks, but with our debugger rewrite we plan on coming in with a *far* better test suite. Until then this probably should live in a different folder to appease the timeout gods...

::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
@@ +112,5 @@
> +  });
> +
> +  let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});
> +  let closePromise;
> +  yield new Promise(onRun => {

Is there much point in promises here? It makes the code a lot more confusing. Instead you could do:

BrowserToolboxProcess.init(
  () => {
    ok(true, "Browser toolbox process just closed");
    clearInterval(interval);
  },
  () => ok(true, "Browser toolbox started\n")
);
Attachment #8743204 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #8)
> Comment on attachment 8743204 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8743204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (sorry, was off most of last week)
> 
> What changed to allow this? I thought there was a technical limitation that
> make testing the browser toolbox hard.

Nothing changed. I just made this non-obvious test case which pipes JS through a process environment variable! We are not going to run all tests against the browser toolbox, but just going to have a set of meaningful test that check for the typical browser toolbox regressions.
Like this one, to check that we do not end in debugger internals while stepping.
(In reply to James Long (:jlongster) from comment #8)
> ::: devtools/client/framework/test/browser_browser_toolbox_debugger.js
> @@ +112,5 @@
> > +  });
> > +
> > +  let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});
> > +  let closePromise;
> > +  yield new Promise(onRun => {
> 
> Is there much point in promises here? It makes the code a lot more
> confusing. Instead you could do:
> 
> BrowserToolboxProcess.init(
>   () => {
>     ok(true, "Browser toolbox process just closed");
>     clearInterval(interval);
>   },
>   () => ok(true, "Browser toolbox started\n")
> );

Now that I re-read this code, I also find this code confusing.
But you suggestion doesn't work as I do have to wait for callbacks to be called before finishing the tests. So I need to yield on some promises. It would certainly look less indented and may be easier to read if I use devtools promises instead of DOM one, but I thought it was better to use DOM one in new code?
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> 
> Now that I re-read this code, I also find this code confusing.
> But you suggestion doesn't work as I do have to wait for callbacks to be
> called before finishing the tests. So I need to yield on some promises. It
> would certainly look less indented and may be easier to read if I use
> devtools promises instead of DOM one, but I thought it was better to use DOM
> one in new code?

I think you're right; the only reason I can think of to use devtools promises is if we still don't make tests fail for unhandled DOM promise rejections. Here you probably should use native promises.

I'll leave it up to you to decide how you want to clean it up; at least a comment describing the workflow would be nice.

In general I don't have much to say about the test because how this works goes over my head :) But it's great to have a test to make sure it works!
Attached patch patch v4Splinter Review
With a comment about the DOM promises and nit addressed.
Attachment #8745391 - Flags: review+
Attachment #8743204 - Attachment is obsolete: true
(In reply to James Long (:jlongster) from comment #11)
> (In reply to Alexandre Poirot [:ochameau] from comment #10)
> > 
> > Now that I re-read this code, I also find this code confusing.
> > But you suggestion doesn't work as I do have to wait for callbacks to be
> > called before finishing the tests. So I need to yield on some promises. It
> > would certainly look less indented and may be easier to read if I use
> > devtools promises instead of DOM one, but I thought it was better to use DOM
> > one in new code?
> 
> I think you're right; the only reason I can think of to use devtools
> promises is if we still don't make tests fail for unhandled DOM promise
> rejections. Here you probably should use native promises.

Bug 1242505 tracks the unhandled rejection tracking in the test harness for DOM Promises.  I typically use Promise.jsm still, since this bug is not yet fixed.
https://hg.mozilla.org/mozilla-central/rev/ed934827beba
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.