952091 - Robocop: Add test to show Title Bar options

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

This bug will cover an automated test for showing the full URL and title in the URL Bar.
MozTrap testcase: https://moztrap.mozilla.org/manage/case/8076/

Comment 1

[Teodora Vermesan (:TeoVermesan)]

Attachment: Title Bar Options

The patch tests the Title Bar Options showing the full URL and title in the URL Bar.
I have run the tests locally on multiple devices and there were no failures.
The try-server is all green: https://tbpl.mozilla.org/?tree=Try&rev=ae31597cba4b



Should it be stand alone or integrated with another test?

Comment 2

[Geoff Brown [:gbrown]]

Sorry, I won't have a chance to look at this until after the holidays. At a glance, I think it would be better to integrate this with testSettingsMenuItems.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 8350566 [details] [diff] [review]
Title Bar Options

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

::: mobile/android/base/tests/testTitleBar.java
@@ +40,5 @@
> +
> +    // Entering settings, changing the options: show title/page address option and verifing the device type because for phone there is an extra back action to exit the settings menu
> +    public void selectOption(String option) {
> +        selectSettingsItem(StringHelper.DISPLAY_SECTION_LABEL, StringHelper.TITLE_BAR_LABEL);
> +        mAsserter.ok(waitForText(StringHelper.TITLE_BAR_LABEL), "Waiting for the pop-up to open", "Pop up with the options was openend");

selectSettingsItem() will click to open the Display settings, which include the settings item "Title bar". It will then select "Title bar" to open the pop-up which is titled "Title bar". I expect the waitForText("Title bar") is waiting for the pop-up to open, but even if it has not opened yet, waitForText will succeed because "Title bar" is on the Display settings page. I think you would wait for something else, like "Show page title" instead.

@@ +42,5 @@
> +    public void selectOption(String option) {
> +        selectSettingsItem(StringHelper.DISPLAY_SECTION_LABEL, StringHelper.TITLE_BAR_LABEL);
> +        mAsserter.ok(waitForText(StringHelper.TITLE_BAR_LABEL), "Waiting for the pop-up to open", "Pop up with the options was openend");
> +        mSolo.clickOnText(option);
> +        mAsserter.ok(waitForText(StringHelper.TITLE_BAR_LABEL), "Waiting to press the option", "The pop-up is dismissed once clicked");

Here there is the reverse problem: waitForText("Title bar") will succeed if the pop-up is open or closed.

@@ +45,5 @@
> +        mSolo.clickOnText(option);
> +        mAsserter.ok(waitForText(StringHelper.TITLE_BAR_LABEL), "Waiting to press the option", "The pop-up is dismissed once clicked");
> +        if (mDevice.type.equals("phone")) {
> +            mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +            mAsserter.ok(waitForText(StringHelper.DISPLAY_SECTION_LABEL), "Waiting to perform one back", "One back performed");

Again..."Display" is found both on the main Settings page and as the title of the Display settings -- you should not use that to distinguish between being on one page or the other.

Comment 4

[Teodora Vermesan (:TeoVermesan)]

Attachment: Title Bar Options

I made the  requested changes and the try server is green: https://tbpl.mozilla.org/?tree=Try&rev=07a1c1fd73a9

I think testSettingsMenuItems is too large. It only verifies that the sections in the menu are present and it doesn't tests its functionality. Is this test too short to be independent? Or should be integrated with another test?

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 8360413 [details] [diff] [review]
Title Bar Options

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

Thanks -- looks good now. It's fine to keep this as a separate test if you like.

::: mobile/android/base/tests/testTitleBar.java
@@ +1,4 @@
> +package org.mozilla.gecko.tests;
> +import android.view.View;
> +import org.mozilla.gecko.*;
> +import java.util.ArrayList;

nit -- I think you can remove the android.view.View and java.util.ArrayList imports

@@ +6,5 @@
> +/**
> + * This patch tests the option that shows the full URL and title in the URL Bar
> + */
> +
> +public class testTitleBar extends PixelTest {

Does this need to be a PixelTest? It is okay if it does, but I think you could inherit from BaseTest.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/478c4361aa0b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/478c4361aa0b