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)
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)
1.27 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Specifically in testBookmarksPanel.assertAllContextMenuOptionsArePresent.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][good first bug]
Comment 1•10 years ago
|
||
Sharing this around - Michael, can you add a few details to this bug, to get somebody started?
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 2•10 years ago
|
||
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)
Updated•10 years ago
|
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java][good first bug] → [lang=java][good first bug]
Comment 3•10 years ago
|
||
Assignee: nobody → shashank
Mentor: michael.l.comella
Attachment #8441706 -
Flags: review?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Updated•10 years ago
|
Mentor: michael.l.comella
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(michael.l.comella)
Reporter | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
Attachment #8441706 -
Attachment is obsolete: true
Attachment #8444696 -
Flags: review?(michael.l.comella)
Reporter | ||
Comment 6•10 years ago
|
||
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-
Reporter | ||
Comment 7•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: shashank → nobody
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
Forgot to add the actual patch, here it is :)
Attachment #8466176 -
Flags: review?(michael.l.comella)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → treemantris
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•10 years ago
|
||
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+
Reporter | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8839fc5e550a
Reporter | ||
Comment 13•10 years ago
|
||
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/
Assignee | ||
Comment 14•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8466176 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
(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
Reporter | ||
Comment 17•10 years ago
|
||
take two: https://tbpl.mozilla.org/?tree=Try&rev=191133441849 When green, you know what to do!
Flags: needinfo?(treemantris)
Assignee | ||
Comment 18•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8444696 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
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]
Comment 20•10 years ago
|
||
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
Reporter | ||
Comment 21•10 years ago
|
||
Thanks Tristan!
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•