1581525 - Stop setting testing flag in debugger's DAMP tests

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Alexandre Poirot [:ochameau]]

Bug 1439369 introduced a new DAMP test, but in order to do that, it started setting the devtools.testing preference to true.

While it makes sense to ensure that this pref is set for most test harnesses, it doesn't for performance one.
Performance tests are special. Firefox build used in DAMP/Talos is actually different from the one used in mochitests. It is a release build, without any test preference/add-ons/...
The only added thing is the DAMP add-on.
We do that in order to better reflect what the final users are going to experience and want to prevent having any special test-only behavior. For example, additional log messages being emitted during tests may make Firefox slightly slower. This will pollute the measurement and if we happen to make that logging code slower/faster, this will have an impact on DAMP while it won't for the end users.
Also, in the particular case of the debugger, this made the test longer to execute. That simple consequence makes it so that it will be harder to track smaller regression as it is always hard to track regression smaller than a few percents. We typically track only regression above 1 to 5 percent, depending on the typical test variance.
So if we switched from running a particular test in 1s up to running it in 10s with the additonal testing logs, we would switch from being to catch 3% of 1s = 30ms regression up to catching only regression of 3% of 10s = 300ms and more.

Comment 1

[David Walsh :davidwalsh]

So I did a quick look around the code and we are still using isTesting(), which is a wrapper for that pref :/

Comment 2

[Julian Descottes [:jdescottes]]

I also logged Bug 1557064 3 months ago for this issue. I'll close my old bug as duplicate, there was no significant activity on the Bug and our summaries are similar :)

Comment 4

[Julian Descottes [:jdescottes]]

Hi David,

So our problem comes from:

    // The first time a popup is rendered, the mouse should be hovered
    // on the token. If it happens to be hovered on whitespace, it should
    // not render anything
    if (!target.matches(":hover") && !isTesting()) {
      return;
    }

Since the debugger test is just simulating a mouseover event, the element doesn't have the hover state.
I think we can still do something to improve the situation :)

One option would be to have another preference called eg isPerformanceTesting that would be checked only here (as in !(isTesting() || isPerformanceTesting())).

Another option (much more fun!) is to rely on the InspectorUtils helpers called addPseudoClassLock/removePseudoClassLock. With this you will be able to add the ":hover" state to your element and the test will pass just fine without setting devtools.testing to true.

Are you interested in trying out either of those approaches? Happy to help with reviews etc...

Comment 5

[David Walsh :davidwalsh]

This looks great but I have a full stack of crucial tasks at the moment so it may be a while before I can get to this.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1581525 - Force hover state in debugger preview test for DAMP and stop forcing devtools.testing

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to David Walsh :davidwalsh from comment #5)

This looks great but I have a full stack of crucial tasks at the moment so it may be a while before I can get to this.

No worries, I'll go forward with a patch then :)
Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbf466aea0e4c79ac3acac3ad3447c66942d7207
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d5b179380428cf69485dda7cfe634b3c308b9aa

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd13bca2703b
Force hover state in debugger preview test for DAMP and stop forcing devtools.testing r=jlast

Comment 9

[Julian Descottes [:jdescottes]]

It mostly seems to speed up close, preview and reload. In https://bugzilla.mozilla.org/show_bug.cgi?id=1556395 I had mentioned that it also regressed stepIn and open, but they don't seem to change after this. Maybe the charts will show more after a few days.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cd13bca2703b