Closed Bug 1351266 Opened 7 years ago Closed 7 years ago

Add tests for the rule preview tooltips (image & font preview)

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jdescottes, Assigned: ochameau)

References

Details

Attachments

(1 file)

Bug 1351266 - Test preview and font tooltips by faking mouse events.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
While reviewing Bug 1320939, we found out that no test was checking that the font and preview tooltips are displayed on mouseover.

These are both quite important features and we should at least have a basic smoke test here.
I'll try to do something about that while I have a regression available.
Assignee: nobody → poirot.alex
This patch only fixes one test. If you agree on this one, I'll refactor the others.
i.e. all the one using assertHoverTooltipOn.

It fails if you comment the line with getTooltip("previewTooltip") from TooltipsOverlay.addToView in bug 1320939.
Comment on attachment 8852550 [details]
Bug 1351266 - Test preview and font tooltips by faking mouse events.

https://reviewboard.mozilla.org/r/124744/#review127340

Ah that makes more sense! 
We did have tests, but since they were calling getTooltip before starting the test they didn't catch the regression.

Nonetheless I think this improvement makes sense and we should go forward with it.

::: devtools/client/inspector/shared/test/browser_styleinspector_tooltip-background-image.js:53
(Diff revision 1)
>  
>    info("Testing that the background-image computed style has a tooltip too");
>    yield testComputedView(view);
>  });
>  
> +function* assertShowTooltip(view, target, name, offsetX = 0, offsetY = 0) {

offsetX and offsetY unused

name is always previewTooltip in this test, maybe no reason to make it an argument?

::: devtools/client/inspector/shared/test/browser_styleinspector_tooltip-background-image.js:92
(Diff revision 1)
>  
>    // Get the background-image property inside the rule view
>    let {valueSpan} = getRuleViewProperty(view, "body", "background-image");
>    let uriSpan = valueSpan.querySelector(".theme-link");
>  
> -  yield assertHoverTooltipOn(view.tooltips.getTooltip("previewTooltip"), uriSpan);
> +  let previewTooltip = yield assertShowTooltip(view, uriSpan, "previewTooltip", 100);

100 not used

::: devtools/client/inspector/shared/test/browser_styleinspector_tooltip-background-image.js:108
(Diff revision 1)
> -
>    // Get the background property inside the rule view
>    let {valueSpan} = getRuleViewProperty(view, ".test-element", "background");
>    let uriSpan = valueSpan.querySelector(".theme-link");
>  
> -  yield assertHoverTooltipOn(view.tooltips.getTooltip("previewTooltip"), uriSpan);
> +  let previewTooltip = yield assertShowTooltip(view, uriSpan, "previewTooltip", 0, 15);

0, 15 not used

::: devtools/client/inspector/shared/test/browser_styleinspector_tooltip-background-image.js:126
(Diff revision 1)
>    info("Now trying to show the preview tooltip");
>    let {valueSpan} = getRuleViewProperty(view, ".test-element", "background");
>    let uriSpan = valueSpan.querySelector(".theme-link");
> -  yield assertHoverTooltipOn(view.tooltips.getTooltip("previewTooltip"), uriSpan);
>  
> +  let previewTooltip = yield assertShowTooltip(view, uriSpan, "previewTooltip", 0, 15);

ditto

::: devtools/client/inspector/shared/test/browser_styleinspector_tooltip-background-image.js:146
(Diff revision 1)
>    let uriSpan = valueSpan.querySelector(".theme-link");
>  
> -  yield assertHoverTooltipOn(view.tooltips.getTooltip("previewTooltip"), uriSpan);
> +  // Scroll to ensure the line is visible as we see the box model by default
> +  valueSpan.scrollIntoView();
>  
> -  let images = panel.getElementsByTagName("img");
> +  let previewTooltip = yield assertShowTooltip(view, uriSpan, "previewTooltip", 100);

ditto
Attachment #8852550 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #4)
> Comment on attachment 8852550 [details]
> Bug 1351266 - Test preview and font tooltips by faking mouse events.
> 
> https://reviewboard.mozilla.org/r/124744/#review127340
> 
> Ah that makes more sense! 
> We did have tests, but since they were calling getTooltip before starting
> the test they didn't catch the regression.

There is that, but also assertHoverTooltipOn was doing weird things.
At the end we weren't sending mouse event. It isn't really clear how we ended up having valid tests at all.
If I got it correctly it was just because assertHoverTooltipOn ended up calling  tooltip._toggle.isValidHoverTarget with the target node specified in test. This was the precise call which may end up updating the tooltip content to the expected one...

> devtools/client/inspector/shared/test/browser_styleinspector_tooltip-
> background-image.js:53
> (Diff revision 1)
> >  
> >    info("Testing that the background-image computed style has a tooltip too");
> >    yield testComputedView(view);
> >  });
> >  
> > +function* assertShowTooltip(view, target, name, offsetX = 0, offsetY = 0) {
> 
> offsetX and offsetY unused

Oh yes, I had so many issues in dispatching correct mouse events... I'll clean that up.

> name is always previewTooltip in this test, maybe no reason to make it an
> argument?

I'm planning to move that to some head.js file in order to use it across all tests currently using assertHoerTooltipOn.
So I'll keep that as-is for this test.
Comment on attachment 8852550 [details]
Bug 1351266 - Test preview and font tooltips by faking mouse events.

I just pushed an update that modifies all tests using assertHoverOnTooltip and isValidTarget.
Attachment #8852550 - Flags: review+ → review?(jdescottes)
Comment on attachment 8852550 [details]
Bug 1351266 - Test preview and font tooltips by faking mouse events.

https://reviewboard.mozilla.org/r/124744/#review132582

LGTM if try is green!

::: devtools/client/inspector/test/head.js:648
(Diff revision 2)
>    }
>  }
>  
>  /**
> - * Given a tooltip object instance (see Tooltip.js), checks if it is set to
> - * toggle and hover and if so, checks if the given target is a valid hover
> + * Given a Tooltip instance, fake a mouse event on the `target` DOM Element
> + * and assert that the tooltip with the given `name` is corretly displayed.

this comment refers to a `name` argument which is not used in the final implementation.
corretly -> correctly

[I guess initially you wanted to have a single implementation for both assertShowTooltip & assertShowPreviewTooltip. 

But since you want the code for getting the tooltip instance to be after the event generation, it makes the mutualization not really worth it.]

::: devtools/client/inspector/test/head.js:657
(Diff revision 2)
> + * @param {DOMElement} target
> + *        The DOM Element on which a tooltip should appear
> + *
> + * @return a promise that resolves with the tooltip object
>   */
> -function isHoverTooltipTarget(tooltip, target) {
> +function* assertShowTooltip(tooltip, target) {

We should rename this method to hint at the fact that the tooltip is shown on mousemove / hover

`assertTooltipShownOnHover` ?

::: devtools/client/inspector/test/head.js:690
(Diff revision 2)
> -  }, () => {
> -    ok(false, "No tooltip is defined on hover of the given element");
>    });
> +  target.dispatchEvent(mouseEvent);
> +
> +  let name = "previewTooltip";

not sure we need to keep the name variable here? (just mentioning it because I suppose this might be a leftover from an older implementation, but you can keep as is if you want)

::: devtools/client/inspector/test/head.js:714
(Diff revision 2)
> + * @param {Tooltip} tooltip
> + *        The tooltip instance
> + * @param {DOMElement} target
> + *        The DOM Element on which a tooltip should appear
> + */
> +function* assertHideTooltip(tooltip, target) {

same comment about the function name, `assertTooltipHiddenOnMouseout` ?
Attachment #8852550 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #9)
> We should rename this method to hint at the fact that the tooltip is shown
> on mousemove / hover
> 
> `assertTooltipShownOnHover` ?

Renamed to that name.

> same comment about the function name, `assertTooltipHiddenOnMouseout` ?

Same.

> ::: devtools/client/inspector/test/head.js:690
> (Diff revision 2)
> > -  }, () => {
> > -    ok(false, "No tooltip is defined on hover of the given element");
> >    });
> > +  target.dispatchEvent(mouseEvent);
> > +
> > +  let name = "previewTooltip";
> 
> not sure we need to keep the name variable here? (just mentioning it because
> I suppose this might be a leftover from an older implementation, but you can
> keep as is if you want)

I kept this to avoid repeating "previewTooltip" in this function as it is used multiple times.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2a45b170d00
Test preview and font tooltips by faking mouse events. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/b2a45b170d00
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: