Closed Bug 1500986 Opened Last year Closed Last year

Migrate old debugger tests


(DevTools :: Debugger, enhancement)

Not set


(firefox65 fixed)

Firefox 65
Tracking Status
firefox65 --- fixed


(Reporter: davidwalsh, Assigned: davidwalsh)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

There are a handful of tests that currently live in the following directory:


These should be moved to:


They will need to be transitioned to the new debugger UI.
Blocks: 1500987
No longer blocks: 1314057
Blocks: 1314057
Test conditionals:

skip-if = (e10s && debug || os == 'win' || verify) || true # bug 1005274

uses-unsafe-cpows = true
skip-if = e10s && debug

uses-unsafe-cpows = true
skip-if = true # Bug 1177730

uses-unsafe-cpows = true
skip-if = e10s && debug

uses-unsafe-cpows = true
skip-if = e10s && debug

skip-if = (e10s && debug) || true # Bug 1486974
:jdescottes:  Once 1 and two go in, I'm going to close this bug and create separate bugs for the remaining tests.  I don't want this to balloon like the other bug.
(In reply to David Walsh :davidwalsh from comment #4)
> :jdescottes:  Once 1 and two go in, I'm going to close this bug and create
> separate bugs for the remaining tests.  I don't want this to balloon like
> the other bug.

Great, I was going to ask for this :)
Comment on attachment 9019681 [details] [diff] [review]

Review of attachment 9019681 [details] [diff] [review]:

We have a bunch of methods duplicated between devtools/client/shared/test/helper_workers.js and devtools/client/debugger/test/mochitest/head.js. 
Most of the time those methods are not even used anymore in the debugger tests.
But we could simply import the helper in debugger's head.js and remove all the code duplication.

Is this planned in another bug? If not can we either do it here or file a bug to track this?

A suggestion for a follow up:
Should we isolate the debugger client tests (browser_dbg) in their own ini file. 
You can have several ini files in the same folder, so we could add browser-debugger-client.ini and have all the debugger client tests in this one.

::: devtools/client/shared/test/helper_workers.js
@@ +257,5 @@
> +  await threadClient.resume();
> +  return threadClient;
> +}
> +
> +function getTargetActorForUrl(client, url) {

switch to async / await maybe?
Attachment #9019681 - Flags: review?(jdescottes) → review+
Comment on attachment 9019833 [details] [diff] [review]

Review of attachment 9019833 [details] [diff] [review]:

Thanks for the patch David!

Probably should have been done in an earlier changeset but we should remove the topmost comment of debugger's browser.ini

  # Tests in this directory are split into two manifests (this and browser2.ini)
  # to facilitate better chunking; see bug 1294489.header

Otherwise a few things to fix, but no need for another round.

::: devtools/client/shared/test/browser.ini
@@ +76,5 @@
>  [browser_cubic-bezier-05.js]
>  [browser_cubic-bezier-06.js]
>  [browser_cubic-bezier-07.js]
> +[browser_dbg_addon-console.js]
> +skip-if = (e10s && debug || os == 'win' || verify) || true # bug 1005274

Sorry for missing this in the review of, but the `|| true` here should be removed.

::: devtools/client/shared/test/browser_dbg_addon-console.js
@@ +10,5 @@
> +  "chrome://mochitests/content/browser/devtools/client/shared/test/helper_addons.js",
> +  this);
> +
> +/* import-globals-from helper_workers.js */
> +Services.scriptloader.loadSubScript(

The only reason we are importing this helper is to get 

var { DebuggerServer } = require("devtools/server/main");
var { DebuggerClient } = require("devtools/shared/client/debugger-client");
var { Toolbox } = require("devtools/client/framework/toolbox");

We should either define it in the test or in helper_addons, otherwise it's not clear why we need a workers helper for an addon test.

::: devtools/client/shared/test/helper_addons.js
@@ +19,5 @@
> + */
> +function getDeferredPromise() {
> +  // Override promise with deprecated-sync-thenables
> +  const promise = require("devtools/shared/deprecated-sync-thenables");
> +  return promise;

I am losing track of why we still need this special promise here, can we file a bug to investigate that?
Attachment #9019833 - Flags: review?(jdescottes) → review+
Attached patch 1500986-02.patchSplinter Review
Attachment #9019833 - Attachment is obsolete: true
Attachment #9020086 - Flags: review+
Keywords: checkin-needed
Pushed by
Migrate old event-listener and worker debugger tests  r=jdescottes
Migrate addon test from old debugger to shared r=jdescottes
Keywords: checkin-needed
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.