901899 - Add testBrowserSearchVisibility

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

To test the visibility behaviour for BrowserSearch.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add testBrowserSearchVisility

Comment 2

[:Margaret Leibovic]

Comment on attachment 786226 [details] [diff] [review]
Add testBrowserSearchVisility

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

Nice, I really like this! I just have one concern about the logic in assertBrowserSearchVisibility

::: mobile/android/base/tests/testBrowserSearchVisibility.java.in
@@ +67,5 @@
> +                final View v = browserSearch.getView();
> +                if (isVisible && v != null && v.getVisibility() == View.VISIBLE)
> +                    return true;
> +
> +                return false;

What if we're trying to assert that BrowserSearch isn't visible (isVisible = false)... Shouldn't we then still return true if (!isVisible && v != null && v.getVisibility() != View.VISIBLE) ? Or is it only not visible if the Fragment is null? At the very least, some more comments here would be good to help clarify this logic, since it can be hard to reason through (at least for me :)

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 786226 [details] [diff] [review]
> Add testBrowserSearchVisility
> 
> Review of attachment 786226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, I really like this! I just have one concern about the logic in
> assertBrowserSearchVisibility
> 
> ::: mobile/android/base/tests/testBrowserSearchVisibility.java.in
> @@ +67,5 @@
> > +                final View v = browserSearch.getView();
> > +                if (isVisible && v != null && v.getVisibility() == View.VISIBLE)
> > +                    return true;
> > +
> > +                return false;
> 
> What if we're trying to assert that BrowserSearch isn't visible (isVisible =
> false)... Shouldn't we then still return true if (!isVisible && v != null &&
> v.getVisibility() != View.VISIBLE) ? Or is it only not visible if the
> Fragment is null? At the very least, some more comments here would be good
> to help clarify this logic, since it can be hard to reason through (at least
> for me :)

I should have probably added a comment about this. What we're testing here is not only that the BrowserSearch fragment's view is not "visible" but also that the fragment itself has been completely removed from the app. If a fragment is present (in the fragment manager) but its view is null, this just means it has been detached but not necessarily removed (which is not what we're testing here).

So, I'll add a comment explaining this for clarity.

Comment 4

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/projects/fig/rev/8a36903511fd

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a36903511fd