Closed Bug 1500986 Opened Last year Closed Last year

Migrate old debugger tests

Categories

(DevTools :: Debugger, enhancement)

enhancement
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: davidwalsh, Assigned: davidwalsh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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

/devtools/client/debugger/test/mochitest

These should be moved to:

/devtools/client/shared/test

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

[browser_dbg_addon-console.js]
skip-if = (e10s && debug || os == 'win' || verify) || true # bug 1005274

[browser_dbg_promises-allocation-stack.js]
uses-unsafe-cpows = true
skip-if = e10s && debug

[browser_dbg_promises-chrome-allocation-stack.js]
uses-unsafe-cpows = true
skip-if = true # Bug 1177730

[browser_dbg_promises-fulfillment-stack.js]
uses-unsafe-cpows = true
skip-if = e10s && debug

[browser_dbg_promises-rejection-stack.js]
uses-unsafe-cpows = true
skip-if = e10s && debug

[browser_dbg_worker-window.js]
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]
1500986-01.patch

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]
1500986-02.patch

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 https://bugzilla.mozilla.org/show_bug.cgi?id=1314057, 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 ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3eb80797aa
Migrate old event-listener and worker debugger tests  r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5d6ad21391
Migrate addon test from old debugger to shared r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca3eb80797aa
https://hg.mozilla.org/mozilla-central/rev/9c5d6ad21391
Status: NEW → RESOLVED
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.