Cleanup the debugger tests

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(20 attachments, 1 obsolete attachment)

18.12 KB, patch
past
: review+
Details | Diff | Splinter Review
1.42 KB, patch
past
: review+
Details | Diff | Splinter Review
1.10 KB, patch
past
: review+
Details | Diff | Splinter Review
2.28 KB, patch
past
: review+
Details | Diff | Splinter Review
1.79 KB, patch
past
: review+
Details | Diff | Splinter Review
2.72 KB, patch
past
: review+
Details | Diff | Splinter Review
46.58 KB, patch
Details | Diff | Splinter Review
1.81 KB, patch
past
: review+
Details | Diff | Splinter Review
20.06 KB, patch
vporof
: review+
Details | Diff | Splinter Review
29.60 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
27.10 KB, patch
past
: review+
Details | Diff | Splinter Review
142.06 KB, patch
past
: review+
Details | Diff | Splinter Review
189.46 KB, patch
past
: review+
Details | Diff | Splinter Review
28.28 KB, patch
past
: review+
Details | Diff | Splinter Review
456.92 KB, patch
past
: review+
Details | Diff | Splinter Review
81.49 KB, patch
past
: review+
Details | Diff | Splinter Review
236.30 KB, patch
past
: review+
Details | Diff | Splinter Review
18.63 KB, patch
past
: review+
Details | Diff | Splinter Review
11.28 KB, patch
past
: review+
Details | Diff | Splinter Review
9.43 KB, patch
msucan
: review+
anton
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
msucan recently pointed out to me that the debugger tests are horror. I sincerely believe that's a gross understatement. Everything is WET (write-everything-twice), there are a million executeSoon's and private methods and properties are accessed everywhere.

Let's make them pretty and a joy to write!
(Assignee)

Updated

5 years ago
Priority: -- → P3
(Assignee)

Updated

4 years ago
Depends on: 886848, 891439
(Assignee)

Comment 1

4 years ago
Created attachment 789110 [details] [diff] [review]
Part 1: Convert the debugger frontend to use the EventEmitter instead of relying on DOM events
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #789110 - Flags: review?(past)
(Assignee)

Comment 2

4 years ago
Created attachment 789113 [details] [diff] [review]
Part 2: Make widgets always return labels and values in the displayed order, to avoid tests failing because of the async way in which items could be added
Attachment #789113 - Flags: review?(past)
(Assignee)

Comment 3

4 years ago
Created attachment 789114 [details] [diff] [review]
Part 3: Make widgets always perform a refresh operation when the selected item is changed, to prevent any consumers from dealing with such scenarios
Attachment #789114 - Flags: review?(past)
(Assignee)

Comment 4

4 years ago
Created attachment 789116 [details] [diff] [review]
Part 4: Clicking on global search results lines (not matches) was broken after 877686, fix it
Attachment #789116 - Flags: review?(past)
(Assignee)

Comment 5

4 years ago
Created attachment 789117 [details] [diff] [review]
Part 5: Add getScopeAtIndex() to make scopes more easily accessible from tests, and fix a few inconsistencies in VariablesView.jsm
Attachment #789117 - Flags: review?(past)
(Assignee)

Comment 6

4 years ago
Created attachment 789118 [details] [diff] [review]
Part 6: Remove the ugly _wasToggled hack from the VariablesView that tests relied on
Attachment #789118 - Flags: review?(past)
(Assignee)

Comment 7

4 years ago
Created attachment 789119 [details] [diff] [review]
Part 7: Normalize formatting for non-test files and rename them to follow a nicer pattern
Attachment #789119 - Flags: review?(past)
(Assignee)

Comment 8

4 years ago
Created attachment 789121 [details] [diff] [review]
Part 8: Rewrite head.js use promises and remove useless cruft
Attachment #789121 - Flags: review?(rcampbell)
Attachment #789121 - Flags: review?(past)
Attachment #789121 - Flags: review?(nfitzgerald)
(Assignee)

Comment 9

4 years ago
Created attachment 789123 [details] [diff] [review]
Part 9: Remove browser_dbg_menustatus.js because it wasn't used anymore
Attachment #789123 - Flags: review?(past)
(Assignee)

Comment 10

4 years ago
More patches will follow tomorrow, after I'm sure that everything looks good on try and I haven't broken anything.
Comment on attachment 789121 [details] [diff] [review]
Part 8: Rewrite head.js use promises and remove useless cruft

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

This is a very welcome change.

::: browser/devtools/debugger/test/head.js
@@ +18,5 @@
> +let { DebuggerPanel } = Cu.import("resource:///modules/devtools/DebuggerPanel.jsm", {});
> +let { BrowserDebuggerProcess } = Cu.import("resource:///modules/devtools/DebuggerProcess.jsm", {});
> +let { DebuggerServer } = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});
> +let { DebuggerClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {});
> +let { SourceEditor } = Cu.import("resource:///modules/source-editor.jsm", {});

oh yeah, that's nice

@@ +90,3 @@
>  
> +  aClient.listTabs(aResponse => {
> +    let tabActor = aResponse.tabs.filter(aGrip => aGrip.url == aUrl).pop();

Nit: Should really be "form" not "grip", but we might be using this incorrectly on the server side as well.

But honestly, if I were to write it, I'd probably use "t" for "tab" here since this is such a small, one-off filter.

Keep it, change it, do whatever you feel.
Attachment #789121 - Flags: review?(nfitzgerald) → review+
Comment on attachment 789110 [details] [diff] [review]
Part 1: Convert the debugger frontend to use the EventEmitter instead of relying on DOM events

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

I don't really see any significant gains from using the EventEmitter API vs. the DOM Event API, but I don't see any downsides either. Did you just prefer the terseness of "on"/"off" and the availability of "once"?

::: browser/devtools/debugger/debugger-controller.js
@@ +17,5 @@
> +// The panel's window global is an EventEmitter firing the following events:
> +const EVENTS = {
> +  // When the debugger's source editor instance finishes loading or unloading.
> +  EDITOR_LOADED: "Debugger:EditorLoaded",
> +  EDITOR_UNLOADED: "Debugger:EditorUnoaded",

"Unoaded"?

@@ +813,5 @@
>          scope.expand();
>        }
>      } while ((environment = environment.parent));
>  
> +    // Signal that scope environments have been shown and commit the current

Typo: mention either "scopes" or "environments".

@@ +816,5 @@
>  
> +    // Signal that scope environments have been shown and commit the current
> +    // variables view hierarchy to briefly flash items that changed between the
> +    // previous and current scope/variables/properties.
> +    window.emit(EVENTS.FETCHED_SCOPES);

Previously we used to trigger a FETCHED_VARIABLES event, not a FETHCED_SCOPES one. Typo?
Attachment #789110 - Flags: review?(past) → review+
Attachment #789113 - Flags: review?(past) → review+
Attachment #789114 - Flags: review?(past) → review+
Attachment #789116 - Flags: review?(past) → review+
Comment on attachment 789117 [details] [diff] [review]
Part 5: Add getScopeAtIndex() to make scopes more easily accessible from tests, and fix a few inconsistencies in VariablesView.jsm

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1087,5 @@
>    // each of these directly onto any scope, variable or property instance.
>    this.eval = aView.eval;
>    this.switch = aView.switch;
>    this.delete = aView.delete;
> +  this.preventDisableOnChage = aView.preventDisableOnChage;

Maybe take this opportunity to fix the typo in the name?
Attachment #789117 - Flags: review?(past) → review+
Attachment #789118 - Flags: review?(past) → review+
Attachment #789123 - Flags: review?(past) → review+
(Assignee)

Comment 14

4 years ago
(In reply to Panos Astithas [:past] from comment #12)

once + easier to pass params around.

> 
> @@ +816,5 @@
> >  
> > +    // Signal that scope environments have been shown and commit the current
> > +    // variables view hierarchy to briefly flash items that changed between the
> > +    // previous and current scope/variables/properties.
> > +    window.emit(EVENTS.FETCHED_SCOPES);
> 
> Previously we used to trigger a FETCHED_VARIABLES event, not a
> FETHCED_SCOPES one. Typo?

I think there should be a clear delimitation between fetching all environments and fetching variables in a scope. This is a win for tests and avoids confusion, since the local scope has all the variables already available.
Comment on attachment 789121 [details] [diff] [review]
Part 8: Rewrite head.js use promises and remove useless cruft

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

Oh yes, this is the part that is truly God's work!

::: browser/devtools/debugger/test/head.js
@@ +220,5 @@
> +  panelWin.on(aEventName, function onEvent(aEventName, ...aArgs) {
> +    info("Debugger event '" + aEventName + "' fired: " + (++count) + " time(s).");
> +
> +    if (count == aEventRepeat) {
> +      ok(true, "Enough '" + aEventName + "' debugger events have been fired.");

In these functions shouldn't we mention the exact number of events for debugging purposes?

@@ +285,5 @@
> +
> +function navigateActiveTabTo(aPanel, aUrl, aWaitForEventName, aEventRepeat) {
> +  try {
> +    return waitForDebuggerEvents(aPanel, aWaitForEventName, aEventRepeat);
> +  } finally {

I find this try/finally clause in the navigate* and reload* functions rather confusing, since we don't really expect exceptions here AFAICT, we are just using it as a way to sequence the block in the "finally" clause after the block in the "try" clause. In that case, why not just do the usual:

return waitForFoo().then(formerFinallyBlock);

This will also avoid returning a promise of the event arguments that were never requested by the caller (and is an implementation detail).

@@ +383,5 @@
> +    return;
> +  }
> +  if (aDebugger instanceof BrowserDebuggerProcess) {
> +    // Nothing to do here yet.
> +    return;

"else if" if you really want to keep this, or we could add it when we have something useful to do here.
Attachment #789121 - Flags: review?(past) → review+
Comment on attachment 789119 [details] [diff] [review]
Part 7: Normalize formatting for non-test files and rename them to follow a nicer pattern

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

This is missing the changes to Makefile.in (minor, I trust you to get them right) but also the changes to the test files (.js) that go along with the changes to the doc files (.html). Since you seem to be changing test code willy nilly, can I get an updated patch to help make sure we don't miss anything important?

You also seem to be moving all script code in HTML files from the head to the document body. I can see how this may be useful against oranges, but I'm afraid it might leave us without any tests that the debugger works in those cases. Let's make sure at least some tests exercise that part.
Attachment #789119 - Flags: review?(past)
(Assignee)

Comment 17

4 years ago
(In reply to Panos Astithas [:past] from comment #16)

Oh, you really haven't even seen half of the patches. :)
(Assignee)

Comment 18

4 years ago
(In reply to Panos Astithas [:past] from comment #15)ugger events have been fired.");
> 
> In these functions shouldn't we mention the exact number of events for
> debugging purposes?
> 

They are mentioned, before in the info call just above the conditional. Or am I misreading your suggestion?

> @@ +285,5 @@
> > +
> > +function navigateActiveTabTo(aPanel, aUrl, aWaitForEventName, aEventRepeat) {
> > +  try {
> > +    return waitForDebuggerEvents(aPanel, aWaitForEventName, aEventRepeat);
> > +  } finally {
> 
> I find this try/finally clause in the navigate* and reload* functions rather
> confusing, since we don't really expect exceptions here AFAICT, we are just
> using it as a way to sequence the block in the "finally" clause after the
> block in the "try" clause. In that case, why not just do the usual:
> 

You're right, I'm using the try/finally blocks to structure flow. "Return this promise and then immediately do something".

> return waitForFoo().then(formerFinallyBlock);
> 
> This will also avoid returning a promise of the event arguments that were
> never requested by the caller (and is an implementation detail).
> 

But that would cause the promise returned by waitForFoo() to never get fulfilled, since its resolution depends strictly on the execution of the formerFinallyBlock.

What would need to be done in order to not use the try/finally block would be something like this:

let deferred = promise.defer();
waitForFoo().then(deferred.resolve);
formerFinallyBlock();
return deferred.promise;

Am I missing something?
(Assignee)

Comment 19

4 years ago
(In reply to Victor Porof [:vp] from comment #18)
> 
> let deferred = promise.defer();
> waitForFoo().then(deferred.resolve);
> formerFinallyBlock();
> return deferred.promise;
> 

...or: 

executeSoon(formerFinallyBlock);
return waitForFoo();
(In reply to Victor Porof [:vp] from comment #18)
> What would need to be done in order to not use the try/finally block would
> be something like this:
> 
> let deferred = promise.defer();
> waitForFoo().then(deferred.resolve);
> formerFinallyBlock();
> return deferred.promise;
> 
> Am I missing something?


Maybe something like:

> let promise = waitForFoo();
> ... do stuff ...
> return promise;
(Assignee)

Comment 21

4 years ago
(In reply to Brandon Benvie [:bbenvie] from comment #20)
> Maybe something like:
> 
> > let promise = waitForFoo();
> > ... do stuff ...
> > return promise;

Yeah, that seems sensible and equivalent.
(Assignee)

Comment 22

4 years ago
Created attachment 790799 [details] [diff] [review]
Addressed comments for Part 8: Rewrite head.js use promises and remove useless cruft

Addressed comments, removed try/finally blocks to use something less gross.
Attachment #790799 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #789121 - Attachment is obsolete: true
Attachment #789121 - Flags: review?(rcampbell)
If it's not too much trouble, it would be awesome if we could get patches that apply to Aurora and Beta too, keeping in mind that Fx24 is going to be an ESR release that we'll be starring for the next year+ :).
(Assignee)

Comment 24

4 years ago
Created attachment 791316 [details] [diff] [review]
Sourcemaps tests
Attachment #791316 - Flags: review?(past)
(Assignee)

Comment 25

4 years ago
Created attachment 791317 [details] [diff] [review]
Blackboxing tests
Attachment #791317 - Flags: review?(past)
(Assignee)

Comment 26

4 years ago
Created attachment 791318 [details] [diff] [review]
Breakpoints tests
Attachment #791318 - Flags: review?(past)
(Assignee)

Comment 27

4 years ago
Created attachment 791339 [details] [diff] [review]
Searching tests
Attachment #791339 - Flags: review?(past)
(Assignee)

Comment 28

4 years ago
Created attachment 791340 [details] [diff] [review]
Stackframes tests
Attachment #791340 - Flags: review?(past)
(Assignee)

Comment 29

4 years ago
Created attachment 791341 [details] [diff] [review]
VariablesView tests
Attachment #791341 - Flags: review?(past)
(Assignee)

Comment 30

4 years ago
Created attachment 791344 [details] [diff] [review]
Location changes, sources switching and sources cache tests
Attachment #791344 - Flags: review?(past)
(Assignee)

Comment 31

4 years ago
Created attachment 791348 [details] [diff] [review]
Remaining debugger tests
Attachment #791348 - Flags: review?(past)
(Assignee)

Comment 32

4 years ago
Created attachment 791351 [details] [diff] [review]
Add a few more debugger tests
Attachment #791351 - Flags: review?(past)
(Assignee)

Comment 33

4 years ago
Created attachment 791353 [details] [diff] [review]
Update Makefile

There were 3 tests previously disabled, 2 of which on Linux and 1 on OS X. I left them disabled for now (although they *do* seem to pass on try), and will attempt to re-enable them in separate bugs.
Attachment #791353 - Flags: review?(past)
Comment on attachment 791353 [details] [diff] [review]
Update Makefile

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

The only check I would suggest for this change is to compare the results from a try run with all of these patches to an fx-team test run and make sure the total number of tests match.
Attachment #791353 - Flags: review?(past) → review+
Comment on attachment 791340 [details] [diff] [review]
Stackframes tests

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

::: browser/devtools/debugger/test/browser_dbg_stack-05.js
@@ +132,5 @@
> +      "Should have no frames after resume.");
> +    ok(isCaretPos(gPanel, 5),
> +      "Editor caret location is correct after resume.");
> +    is(gEditor.getDebugLocation(), -1,
> +      "Editor debugg location is correct after resume.");

s/debugg/debug/
Attachment #791340 - Flags: review?(past) → review+
Comment on attachment 791351 [details] [diff] [review]
Add a few more debugger tests

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

::: browser/devtools/debugger/test/browser_dbg_reload-preferred-script-03.js
@@ +26,5 @@
> +      .then(() => switchToSource(SECOND_URL))
> +      .then(() => testSource(SECOND_URL))
> +      .then(() => switchToSource(FIRST_URL))
> +      .then(() => testSource(FIRST_URL))
> +      .then(() => closeDebuggerAndFinish(gPanel));

Let's be paranoid and add a then(null, onError) here.
Attachment #791351 - Flags: review?(past) → review+
Comment on attachment 791317 [details] [diff] [review]
Blackboxing tests

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

::: browser/devtools/debugger/test/browser_dbg_blackboxing-02.js
@@ +43,5 @@
> +  let promise = waitForSourceAndCaretAndScopes(gPanel, ".html", 21).then(() => {
> +    is(gFrames.itemCount, 3,
> +      "Should only get 3 frames.");
> +    is(gDebugger.document.querySelectorAll(".dbg-stackframe-black-boxed").length, 1,
> +      "And one of them should be the combined black boxed frames.");

frames -> frame
Attachment #791317 - Flags: review?(past) → review+
(Assignee)

Comment 38

4 years ago
Created attachment 793429 [details] [diff] [review]
Use the debugger's event and promise APIs in webconsole and profiler
Attachment #793429 - Flags: review?(mihai.sucan)
Attachment #793429 - Flags: review?(anton)
Comment on attachment 793429 [details] [diff] [review]
Use the debugger's event and promise APIs in webconsole and profiler

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

Looks good. r+!

::: browser/devtools/webconsole/hudservice.js
@@ +473,3 @@
>     */
>    viewSourceInDebugger:
>    function WC_viewSourceInDebugger(aSourceURL, aSourceLine)

Thanks for the cleanup here. Much nicer.

@@ +483,5 @@
> +    let showSource = ({ DebuggerView }) => {
> +      if (DebuggerView.Sources.containsValue(aSourceURL)) {
> +        DebuggerView.setEditorLocation(aSourceURL, aSourceLine);
> +        return;
> +      }

nit: instead of |return|here why not |} else { selectTool() then viewSource() }| ?

@@ +495,5 @@
> +    let debuggerAlreadyOpen = toolbox.getPanel("jsdebugger");
> +    toolbox.selectTool("jsdebugger").then(({ panelWin: dbg }) => {
> +      if (debuggerAlreadyOpen) {
> +        showSource(dbg);
> +      } else {

nit: }\nelse {
Attachment #793429 - Flags: review?(mihai.sucan) → review+
Comment on attachment 793429 [details] [diff] [review]
Use the debugger's event and promise APIs in webconsole and profiler

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

LGTM.
Attachment #793429 - Flags: review?(anton) → review+
Blocks: 860349
Comment on attachment 791316 [details] [diff] [review]
Sourcemaps tests

Nick is more familiar with these tests.
Attachment #791316 - Flags: review?(past) → review?(nfitzgerald)
Comment on attachment 791316 [details] [diff] [review]
Sourcemaps tests

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

LGTM
Attachment #791316 - Flags: review?(nfitzgerald) → review+
Comment on attachment 791318 [details] [diff] [review]
Breakpoints tests

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

::: browser/devtools/debugger/test/browser_dbg_breakpoints-actual-location.js
@@ +55,5 @@
> +    is(aBreakpointClient.requestedLocation.line, 4,
> +      "Requested location line is correct");
> +
> +    is(aBreakpointClient.location.url, gSources.selectedValue, "URL is the same");
> +    is(aBreakpointClient.location.line, 6, "Line number is new");

You are making these two checks twice, here and above. Copy & paste mix-up?

::: browser/devtools/debugger/test/browser_dbg_breakpoints-contextmenu.js
@@ +25,5 @@
> +      .then(() => performTestWhilePaused())
> +      .then(() => resumeDebuggerThenCloseAndFinish(gPanel))
> +      .then(null, aError => {
> +        ok(false, "Got an error: " + aError.message + "\n" + aError.stack);
> +      });

Minor simplification, but in these builder-pattern invocations you could do .then(foo) when you don't care about passing a specific parameter to foo. Your call of course.

Also, I know how tiresome this patch has been, so I am not going to mention Task.spawn here.

::: browser/devtools/debugger/test/browser_dbg_breakpoints-new-script.js
@@ +30,4 @@
>  
> +    gPanel.addBreakpoint({ url: TAB_URL, line: 20 }).then(() => {
> +      testResume();
> +    });

.then(testResume);
Attachment #791318 - Flags: review?(past) → review+
So this is way harder than it could be, so let me describe the problem now, before I dive in again tomorrow to finish the review.

The conventional wisdom for good patch management says that we never make both functional and style changes in the same patch. That is because the less important style changes confuse the reader trying to detect the functional changes and build a mental model about the modified architecture. This is not a rule I am really attached to, because the occasional style nit doesn't cost me much in review overhead, so I've been (and still am) breaking it occasionally.

The problem with this bug though is that a lot of style changes are combined with a lot of modified APIs due to the events -> promises change. The style changes include splitting long lines to shorter ones, combining short lines to a single one, renaming arguments form "foo" to "aFoo" (the old Firefox style), switching between equivalent APIs (e.g. Array.forEach -> for..of), substituting one variable for an equivalent one (e.g. gDebuggee.window -> gDebuggee), and many more.

Granted, all of these changes considered on their own merit are arguably right, and the resulting files are undoubtedly simpler to understand, but the problem is that they affect a huge number of files. Reviewing each one file requires to keep in mind the change matrix at all times, but unfortunately the human mind can only keep 7 items in its memory cache, and cache misses require consulting other patches, which in turn leads to week-long review cycles :-)

So I'm going to do my best in this case, but what I would find really helpful next time is to either avoid doing style changes along with sweeping functional ones, or to keep them in separate patches so that reviewing them can be done independently. For short patches, or huge patches of one kind that only contain a handful of changes of the other kind, I wouldn't really mind if you chose to combine them.

Thanks!
Comment on attachment 791339 [details] [diff] [review]
Searching tests

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

::: browser/devtools/debugger/test/browser_dbg_search-global-01.js
@@ +24,5 @@
>  
> +    waitForSourceAndCaretAndScopes(gPanel, "-02.js", 6)
> +      .then(() => firstSearch())
> +      .then(() => secondSearch())
> +      .then(() => clearSearch())

If you care:

.then(foo)
.then(bar)

(here and elsewhere)
Attachment #791339 - Flags: review?(past) → review+
Comment on attachment 791344 [details] [diff] [review]
Location changes, sources switching and sources cache tests

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

Looks good.
Attachment #791344 - Flags: review?(past) → review+
Comment on attachment 791348 [details] [diff] [review]
Remaining debugger tests

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

::: browser/devtools/debugger/test/browser_dbg_debugger-statement.js
@@ +50,4 @@
>  
>    // Now attach and resume...
> +  gClient.request({ to: aResponse.threadActor, type: "attach" }, () => {
> +    gClient.request({ to: aResponse.threadActor, type: "resume" }, () => {

You could modernize this test if you wanted with gClient.attachThread & gClient.resume.

@@ +62,5 @@
> +function testDebuggerStatement([aGrip, aResponse]) {
> +  let deferred = promise.defer();
> +
> +  gClient.addListener("paused", (aEvent, aPacket) => {
> +    gClient.request({ to: aResponse.threadActor, type: "resume" }, () => {

Same here.

::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-04.js
@@ -2,5 @@
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */
> -
> -/**
> - * Make sure that the property view filter prefs work properly.

Why was this removed?

::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-07.js
@@ -2,5 @@
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */
> -
> -/**
> - * Make sure that the property view correctly filters nodes.

What about this one?
Attachment #791348 - Flags: review?(past) → review+
Comment on attachment 791341 [details] [diff] [review]
VariablesView tests

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

Looks good.
Attachment #791341 - Flags: review?(past) → review+
Phew, I'm done here!
\o/
(Assignee)

Updated

4 years ago
Blocks: 881219
(Assignee)

Updated

4 years ago
Blocks: 894311
(Assignee)

Updated

4 years ago
Blocks: 843019
(Assignee)

Updated

4 years ago
Blocks: 876633
(Assignee)

Comment 51

4 years ago
(In reply to Panos Astithas [:past] from comment #47)
> 
> Why was this removed?
> 

Those two were redundant checks that didn't exercrise any different/relevant/interesting behavior. I carefully looked into the other tests to make sure.

(In reply to Panos Astithas [:past] from comment #45)

> If you care:
> 
> .then(foo)
> .then(bar)
> 
> (here and elsewhere)

Yup, I agree this looks better. It'd look even better with some yields inside a Task, but oh man, I can't even begin to express how much don't care at this point :)
(Assignee)

Comment 52

4 years ago
(In reply to Panos Astithas [:past] from comment #43)
> Comment on attachment 791318 [details] [diff] [review]
> Breakpoints tests
> 
> You are making these two checks twice, here and above. Copy & paste mix-up?
> 

Not really the same checks. The first couple verifies client.requestedLocation, while the second one verifies client.location.
(In reply to Victor Porof [:vp] from comment #52)
> (In reply to Panos Astithas [:past] from comment #43)
> > You are making these two checks twice, here and above. Copy & paste mix-up?
> > 
> 
> Not really the same checks. The first couple verifies
> client.requestedLocation, while the second one verifies client.location.

I'm actually referring to lines 48-51, not 53-56. They look like identical checks to the ones in lines 58-59, albeit with different messages.
Blocks: 862627
(Assignee)

Comment 54

4 years ago
(In reply to Panos Astithas [:past] from comment #53)

You're right. Fixed.
(Assignee)

Comment 55

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/12928639d2bf
https://hg.mozilla.org/integration/fx-team/rev/b4f56e9c5f96
https://hg.mozilla.org/integration/fx-team/rev/4c1d5a059c6f
https://hg.mozilla.org/integration/fx-team/rev/9cc32e5a67ea
https://hg.mozilla.org/integration/fx-team/rev/ecdd787c9eff
https://hg.mozilla.org/integration/fx-team/rev/1757ae16bf8a
https://hg.mozilla.org/integration/fx-team/rev/0cb94386602a
https://hg.mozilla.org/integration/fx-team/rev/04c318f79c84
https://hg.mozilla.org/integration/fx-team/rev/1f66a755177f
https://hg.mozilla.org/integration/fx-team/rev/c55396293b49
https://hg.mozilla.org/integration/fx-team/rev/647b5d5dc081
https://hg.mozilla.org/integration/fx-team/rev/708bac7a4c99
https://hg.mozilla.org/integration/fx-team/rev/0d004fdd1fdb
https://hg.mozilla.org/integration/fx-team/rev/e721f3ee92bf
https://hg.mozilla.org/integration/fx-team/rev/4d259d8db441
https://hg.mozilla.org/integration/fx-team/rev/4559ea542aa9
https://hg.mozilla.org/integration/fx-team/rev/77b93e70c30a
https://hg.mozilla.org/integration/fx-team/rev/39471a717494
https://hg.mozilla.org/integration/fx-team/rev/5f626c9dd066
https://hg.mozilla.org/integration/fx-team/rev/862a066d90d5
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/12928639d2bf
https://hg.mozilla.org/mozilla-central/rev/b4f56e9c5f96
https://hg.mozilla.org/mozilla-central/rev/4c1d5a059c6f
https://hg.mozilla.org/mozilla-central/rev/9cc32e5a67ea
https://hg.mozilla.org/mozilla-central/rev/ecdd787c9eff
https://hg.mozilla.org/mozilla-central/rev/1757ae16bf8a
https://hg.mozilla.org/mozilla-central/rev/0cb94386602a
https://hg.mozilla.org/mozilla-central/rev/04c318f79c84
https://hg.mozilla.org/mozilla-central/rev/1f66a755177f
https://hg.mozilla.org/mozilla-central/rev/c55396293b49
https://hg.mozilla.org/mozilla-central/rev/647b5d5dc081
https://hg.mozilla.org/mozilla-central/rev/708bac7a4c99
https://hg.mozilla.org/mozilla-central/rev/0d004fdd1fdb
https://hg.mozilla.org/mozilla-central/rev/e721f3ee92bf
https://hg.mozilla.org/mozilla-central/rev/4d259d8db441
https://hg.mozilla.org/mozilla-central/rev/4559ea542aa9
https://hg.mozilla.org/mozilla-central/rev/77b93e70c30a
https://hg.mozilla.org/mozilla-central/rev/39471a717494
https://hg.mozilla.org/mozilla-central/rev/5f626c9dd066
https://hg.mozilla.org/mozilla-central/rev/862a066d90d5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Depends on: 916391
For future reference, please make sure the patches in the bug as well as the landed patches have a consistent numbering.
Blocks: 825738
Blocks: 852006
Blocks: 853058
Blocks: 855811
Blocks: 858778
Blocks: 869144
Blocks: 871713
Blocks: 885983
Blocks: 886058
Blocks: 887363
Blocks: 888087
Blocks: 891176
Blocks: 903231
Blocks: 903301
Blocks: 903303
Blocks: 903750
Blocks: 905092
Blocks: 906459
Blocks: 916591
Blocks: 919161
Blocks: 885967
Blocks: 780198
(Assignee)

Updated

4 years ago
See Also: → bug 968727
You need to log in before you can comment on or make changes to this bug.