Closed Bug 917188 Opened 7 years ago Closed 7 years ago

Use a more deterministic way to write the browser_webconsole_view_source.js test

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: past, Assigned: msucan)

Details

Attachments

(1 file)

This test makes sure that when we click on a source link in the web console that corresponds to a script that has been garbage-collected, view-source will open instead of the debugger. Since the behavior and the test are GC-dependent, bug 916995 added a Cu.forceGC() call to make it more explicit and less prone to oranges. As discussed with Jim on IRC, we should be writing the test in a way that would guarantee it will pass regardless of whether the garbage collector decides to kick at some particular time.

Jim's idea was to make the ThreadActor configurable from tests so that it can lie about a script being present in the debuggee. Since this is a browser chrome test, we need a way to get to the thread actor from the client. Luckily we already have such a method via the _serverConnection property in the client transport object:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#482

I propose we add an _ignoreScripts flag on the thread actor and have the test set it to true, so that the onSources() request handler can consult it and return an empty source list for this test.

I'm filing this bug in the debugger component as most of the necessary changes need to be made to the script.js file.
Priority: -- → P3
I propose a simpler approach that doesn't involve changes in the webconsole/debugger code: the test can override the DebuggerView.Sources.containsValue() to lie. This is what I do with prompt() calls or openInTab() calls, and so on.
I suggested something similar yesterday, to have the console use a fabricated URL that was guaranteed not to be found, but I like your idea better than mine. Jim made the point that we should have the test actually exercise the code path we care about, but in this case, I believe it's just the web console's code path that we want to test.
(In reply to Panos Astithas [:past] from comment #2)
> I suggested something similar yesterday, to have the console use a
> fabricated URL that was guaranteed not to be found, but I like your idea
> better than mine. Jim made the point that we should have the test actually
> exercise the code path we care about, but in this case, I believe it's just
> the web console's code path that we want to test.

Sure --- if that's the intent of the test, then 'mocking' the absence of the script at that level could make sense.
Yes, that is the intent of the test - check that the web console opens view source, when the script is not available in the debugger.
This should work.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c12d7ac5cc42

Should we ask bz to also test this patch with his DOM patches?
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #806856 - Flags: review?(past)
Comment on attachment 806856 [details] [diff] [review]
bug-917188-1-fx-team.diff

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

(In reply to Mihai Sucan [:msucan] from comment #5)
> Should we ask bz to also test this patch with his DOM patches?

Yes, that's a good idea as his main patch seems not to have landed yet.
Attachment #806856 - Flags: review?(past)
Attachment #806856 - Flags: review+
Attachment #806856 - Flags: feedback?(bzbarsky)
Attachment #806856 - Flags: feedback?(bzbarsky) → feedback+
Thank you Panos and Boris!
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a74dbe5b4dae
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a74dbe5b4dae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.