Closed Bug 1023652 Opened 10 years ago Closed 10 years ago

Assert the "Share" option is not visible in the context menu of non-shareable links in testBookmarksPanel

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: mcomella, Assigned: treemantris, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 3 obsolete files)

Specifically in testBookmarksPanel.assertAllContextMenuOptionsArePresent.
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][good first bug]
Sharing this around - Michael, can you add a few details to this bug, to get somebody started?
Flags: needinfo?(michael.l.comella)
In testBookmarksPanel.assertAllContextMenuOptionsArePresent, we verify that all of the context menu options are present on a regular link, and that all but "Share" are visible on non-shareable links. However, we can do better by verifying that "Share" does not exist (rather than just skipping the "Share" menu option - see [1]).

Instead of calling `continue`, assert that the value does not exist!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmarksPanel.java?rev=baf23870f2ea#108
Flags: needinfo?(michael.l.comella)
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java][good first bug] → [lang=java][good first bug]
Assignee: nobody → shashank
Mentor: michael.l.comella
Attachment #8441706 - Flags: review?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Mentor: michael.l.comella
Flags: needinfo?(michael.l.comella)
Comment on attachment 8441706 [details] [diff] [review]
BUG 1023652 -  Assert that the "Share" option is not visible in the context men$

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

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +105,5 @@
>          openBookmarkContextMenu(nonShareableURL);
>          for (String contextMenuOption : StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS) {
>              // This link is not shareable: skip the "Share" option.
>              if ("Share".equals(contextMenuOption)) {
> +		mAsserter.ok(true,

This assertion can never fail and thus isn't very useful - we need to assert that "Share" is actually not visible in the context menu.
Attachment #8441706 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8444696 [details] [diff] [review]
BUG 1023652 -  Assert that the "Share" option is not visible in the context menu of non-shareable links in testBookmarksPanel r=mcomella

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

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +111,5 @@
>          openBookmarkContextMenu(nonShareableURL);
>          for (String contextMenuOption : StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS) {
>              // This link is not shareable: skip the "Share" option.
>              if ("Share".equals(contextMenuOption)) {
> +		mAsserter.ok(mSolo.searchText(contextMenuOption),

mSolo.searchText should not find "Share" in the text and thus return false here, right? This means this assertion will fail. Did you have an opportunity to test these changes? Note that this is not the same as just running the browser, you need to explicitly build and run our Robocop test framework [1] in addition.

Additionally, I would prefer if this check was outside the loop - I think it's a little easier to read as it does a better job of separating concerns. Let me know if you disagree.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop
Attachment #8444696 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8444696 [details] [diff] [review]
BUG 1023652 -  Assert that the "Share" option is not visible in the context menu of non-shareable links in testBookmarksPanel r=mcomella

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

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +111,5 @@
>          openBookmarkContextMenu(nonShareableURL);
>          for (String contextMenuOption : StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS) {
>              // This link is not shareable: skip the "Share" option.
>              if ("Share".equals(contextMenuOption)) {
> +		mAsserter.ok(mSolo.searchText(contextMenuOption),

Note also that this list is scrollable, so one of the options may not be visible off-screen. We should scroll through the list (likely using methods such as Solo.scroll*) to ensure the option is not present in any part of the list.
Assignee: shashank → nobody
Mind if I pick this up? I'm new to Mozilla and this looks like a nice simple one to start with. The only thing I'm not sure about is how to scroll the Solo list, and why .searchText() isn't sufficient to check existence/non-existence of an entry.
Regarding the potential scrolling issue, the Java Docs for Solo.searchText state that it "Will automatically scroll when needed" (https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Solo.java#L620), so that should be a non-issue. Let me know what you think.

Thanks.
Attached patch shareTest.patch (obsolete) — Splinter Review
Forgot to add the actual patch, here it is :)
Attachment #8466176 - Flags: review?(michael.l.comella)
Assignee: nobody → treemantris
Status: NEW → ASSIGNED
Comment on attachment 8466176 [details] [diff] [review]
shareTest.patch

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

Sorry for the delay and thanks for your help!

> Regarding the potential scrolling issue, the Java Docs for Solo.searchText state
> that it "Will automatically scroll when needed"

Nice catch, I didn't see that before.

However, since I was a little skeptical, I skimmed the source and `searchText` is actually fairly fragile. It only scrolls the "freshest view" [1] which is the view that is most recently drawn - this can change by Android version, device customizations, or maybe even a race of what happened to be drawn. This can also change when code affecting the presentation of the background changes.

That being said, it seems to work well enough and we already rely on it in this method so works. Just add a comment explaining why this fragile in the code, just in case it causes problems later.

[1]: https://github.com/RobotiumTech/robotium/blob/robotium-4.3.1/robotium-solo/src/main/java/com/jayway/android/robotium/solo/ViewFetcher.java#L341

> (https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Solo.java#L620),

Careful when reading the robotium source and APIs online - we only use version 4.3.1 in the tree! [2]

[2]: https://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Makefile.in?rev=b578fd863c13#12
Attachment #8466176 - Flags: review?(michael.l.comella) → feedback+
Tristan, the link in comment 12 is to a run of our test suite on our "try" servers. If it goes green, your change has passed the test suite. Note that any changes checked into our version control with the checkin-needed keyword need to have an associated try push with them.

Since the update to your patch will likely only include a comment change, this run should be sufficient to act as that run.

By default, you must apply to receive commit access to our test servers. Once you've fixed a few more bugs, you can request access. [2]

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
[2]: https://www.mozilla.org/hacking/committer/
Attached patch shareTest.patchSplinter Review
Thanks Michael, I've added the comment and updated the patch. The change appears to have passed the test suite, what are the next steps for this bug?
Attachment #8468387 - Flags: review?(michael.l.comella)
Comment on attachment 8468387 [details] [diff] [review]
shareTest.patch

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

I'm not going to make you got through another review cycle for this patch, but I put an educational tidbit into the comments below.

From here, add the `checkin-needed` keyword and you're all set - someone will check in the patch for you! Note again that any time the `checkin-needed` keyword is used, there needs to be a try run associated with the patch, as per the one in comment 12.

Thanks for your help!

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +120,5 @@
>                      contextMenuOption + " is present");
>          }
>  
> +        // The use of Solo.searchText is potentially fragile as It will only
> +        // scroll the most recently drawn view. Works fine for now though.

nit: It -> it

nit: You don't really need to say "Works fine for now." because if it didn't, why are we committing the code? :P
Attachment #8468387 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
(In reply to Michael Comella (:mcomella) from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=8839fc5e550a

Looks like somehow the builds and tests never ran on this push :(
Keywords: checkin-needed
take two: https://tbpl.mozilla.org/?tree=Try&rev=191133441849

When green, you know what to do!
Flags: needinfo?(treemantris)
Ok looks like it's gone green, unless I've read the results wrong. Should be good to check-in.
Flags: needinfo?(treemantris)
Keywords: checkin-needed
Attachment #8444696 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/4bf1e7cab1af
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4bf1e7cab1af
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.