1414460 - Add unit test for getAllFlexbox method of the LayoutActor

Status: RESOLVED WONTFIX

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1414460 - Add unit test for getAllFlexbox method of the LayoutActor.

Review commit: https://reviewboard.mozilla.org/r/198144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198144/

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8926914 [details]
Bug 1414460 - Add unit test for getAllFlexbox method of the LayoutActor.

https://reviewboard.mozilla.org/r/198144/#review203702

Looks good, thanks. I think we miss a few test cases, and then we should be good.

::: devtools/server/tests/browser/browser_layout_getAllFlexbox.js:15
(Diff revision 1)
> +  let { client, walker, layout } = yield initLayoutFrontForUrl(MAIN_DOMAIN +
> +    "flexbox.html");
> +  let flexboxes = yield layout.getAllFlexbox(walker.rootNode, true);
> +  let flexbox = flexboxes[0];
> +
> +  is(flexboxes.length, 1, "One flexbox was returned.");

This is a simple test, it's good, but ideally we would test every possible way for the method to run: 0 containers, 1 container, many containers, missing rootNode, traverseFrame true and false, etc.

::: devtools/server/tests/browser/browser_layout_getAllFlexbox.js:17
(Diff revision 1)
> +  let flexboxes = yield layout.getAllFlexbox(walker.rootNode, true);
> +  let flexbox = flexboxes[0];
> +
> +  is(flexboxes.length, 1, "One flexbox was returned.");
> +
> +  info("Get the grid container node front.");

s/grid/flex

::: devtools/server/tests/browser/browser_layout_simple.js:7
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> -// Simple checks for the LayoutActor and GridActor
> +// Simple checks for the LayoutActor and GridActor.

And FlexBoxActor I guess.

::: devtools/server/tests/browser/browser_layout_simple.js:19
(Diff revision 1)
> +  ok(layout.getAllFlexbox, "The getAllFlexbox method exists");
>    ok(layout.getAllGrids, "The getAllGrids method exists");
>  
>    let didThrow = false;
>    try {
> -    yield layout.getGrids(null);
> +    yield layout.getAllFlexbox(null);

Why don't we do this anymore for grid? We should not remove this test case, but add one for flexbox instead.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Patrick Brosset <:pbro> from comment #2)
> ::: devtools/server/tests/browser/browser_layout_simple.js:19
> (Diff revision 1)
> > +  ok(layout.getAllFlexbox, "The getAllFlexbox method exists");
> >    ok(layout.getAllGrids, "The getAllGrids method exists");
> >  
> >    let didThrow = false;
> >    try {
> > -    yield layout.getGrids(null);
> > +    yield layout.getAllFlexbox(null);
> 
> Why don't we do this anymore for grid? We should not remove this test case,
> but add one for flexbox instead.

getGrids is actually not a public method hence why it was removed.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #3)
> getGrids is actually not a public method hence why it was removed.
Ok. So I guess getAllGrids should be tested then, just like you test getAllFlexbox here. I don't think we test the getAllGrids(null) case now.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

We never went about adding a getAllFlexbox method so closing this.