Closed Bug 1434907 Opened 6 years ago Closed 6 years ago

Remove dependency to hidden.html in test_click.py, test_legacy_mouse_action.py, and test_visibility.py

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: akk5597, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=py])

User Story

For steps in how to get started please consult our documentation for new users:
https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

In the case of questions you can find us on IRC in the #ateam channel.

Attachments

(1 file, 10 obsolete files)

[marionette] Remove dependency to hidden.html in unit tests
5.71 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
No longer depends on: 1423661
Blocks: 1423638
Could I be assigned this bug? It's my first one! I'll look into it this weekend.
Flags: needinfo?(hskupin)
Sure, just get started. Once you uploaded the patch, I'm happy to assign this bug to you. Oh, and be welcome! :)
Flags: needinfo?(hskupin)
Raymond, are you still interested to work on this bug? If not someone else wants to pick this up. Thanks.
Flags: needinfo?(paarthurnax)
I am still interested in this bug. I've got some time this week so I should have it done in the next few days. Cheers :)
Flags: needinfo?(paarthurnax) → needinfo?(hskupin)
Sound good. Thanks! Let me know if there are problems.
Flags: needinfo?(hskupin)
Hey, I've made the changes and ran the tests again. Everything is good but I am unsure how to submit my changes to you.
Flags: needinfo?(hskupin)
The changes you have made need to be committed first. Given that the changes are all related to each other a single commit has to be preserved. So you can commit with `hg --amend`. If you already did a `hg commit` you have to rewrite the history of the commits. For that please see our documentation: 

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews
Flags: needinfo?(hskupin)
Comment on attachment 8954484 [details]
added changes for bug 1434907

https://reviewboard.mozilla.org/r/223546/#review230570

::: browser/app/nsBrowserApp.cpp:169
(Diff revision 1)
>  Bootstrap::UniquePtr gBootstrap;
>  
>  static int do_main(int argc, char* argv[], char* envp[])
>  {
> +
> +  std::cout << "me: MOM GET THE CAMERA \n mom: Why son... \n me: I DID IT\n";

Not sure for what this is for, but it doesn't belong to this bug. Same for the other commits.
Attachment #8954484 - Flags: review-
Summary: Remove dependency to hidden.html in test_click.py, test_mouse_action.py, and test_visibility.py → Remove dependency to hidden.html in test_click.py, test_legacy_mouse_action.py, and test_visibility.py
Is this issue still open to claim to work on? If yes, I'd like to fix it.
Thank you!
Flags: needinfo?(hskupin)
Unless we hear something different from Raymond, he is working on this bug. Pooja, feel free to pick a different dependency from bug 1423638.
Assignee: nobody → paarthurnax
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
I just wanted to give you a status update. I'm bogged down with mid-terms and assignments at the moment. But I will, at the latest have something better for you by Friday. Let me know if this timeframe doesn't work. Cheers :)
Flags: needinfo?(hskupin)
Short delays are generally not a problem. Only let me know when you aren't able at all to continue. Thanks!
Flags: needinfo?(hskupin)
Attachment #8954485 - Attachment is obsolete: true
Attachment #8954486 - Attachment is obsolete: true
Attachment #8954487 - Attachment is obsolete: true
Hey Henrik,

I've collapsed my changes into one commit and removed the extraneous stuff. I hope this works!
I'm impressed that my other changes were automatically obsoleted :)
Flags: needinfo?(hskupin)
(In reply to Raymond Gao from comment #19)
> I've collapsed my changes into one commit and removed the extraneous stuff.
> I hope this works!
> I'm impressed that my other changes were automatically obsoleted :)

Thanks. In the future please set the review flag on the attachment inside of mozreview instead of a needinfo.
Flags: needinfo?(hskupin)
Attachment #8954484 - Flags: review?(ato)
Attachment #8954484 - Flags: review?(ato)
Comment on attachment 8954484 [details]
added changes for bug 1434907

https://reviewboard.mozilla.org/r/223546/#review230570

> Not sure for what this is for, but it doesn't belong to this bug. Same for the other commits.

This was some left over stuff while I was trying to get firefox to helloworld. It's been removed in the latest patch.
(In reply to Raymond Gao from comment #22)
> This was some left over stuff while I was trying to get firefox to
> helloworld. It's been removed in the latest patch.

Thanks. And please do not forget to set the review flag once an update of the patch is ready. Can you try to do it yourself?
Flags: needinfo?(paarthurnax)
Comment on attachment 8954484 [details]
added changes for bug 1434907

https://reviewboard.mozilla.org/r/223546/#review235016

Setting review flag manually for reviewer.
Attachment #8954484 - Flags: review?(paarthurnax)
Flags: needinfo?(paarthurnax)
Attachment #8954484 - Flags: review?(paarthurnax) → review?(hskupin)
Comment on attachment 8954484 [details]
added changes for bug 1434907

https://reviewboard.mozilla.org/r/223546/#review235734

Raymond, thank you for the updated patch! Now it clearly looks better. I checked the changes you did, and those look fine. But please fix the remaining issue as layed out inline. Thanks!

::: commit-message-05fed:1
(Diff revision 2)
> +added changes for bug 1434907

Please follow this style for the summary of the commit: `Bug XYZ - Summary of changes.`

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:107
(Diff revision 2)
>          self.marionette.find_element(By.LINK_TEXT, "333333").click()
>          self.marionette.find_element(By.ID, "testDiv")
>          self.assertEqual(self.marionette.title, "Marionette Test")
>  
>      def test_clicking_an_element_that_is_not_displayed_raises(self):
> -        test_html = self.marionette.absolute_url("hidden.html")
> +        test_html = inline("<div id='parent' hidden> <div id='child'>My parent is hidden so you can't see me</div></div>")

So some information when using inline HTML content:

1) Try to reduce the content as much as possible. Means here we do not need the longish text of the child div. Maybe just put `foo` in it.
2) The parent div has an id which is not used at all in this test. So remove it please.
3) If space permits (should be the case here) directly add the inline code to the `navigate` call.

::: testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py:40
(Diff revision 2)
>      def test_clicking_element_out_of_view_succeeds(self):
>          # The action based click doesn"t check for visibility.
> -        test_html = self.marionette.absolute_url("hidden.html")
> -        self.marionette.navigate(test_html)
> +        self.marionette.navigate(inline("""
> +        <div id='parent' hidden>
> +            <div id='child'>My parent is hidden so you can't see me</div>
> +        </div>"""))

This is the exact same HTML content as for the above test, but claims to test an element which is out of view. So this test is actually broken, and needs to be fixed.

You will have to find some simply HTML/CSS content which pushes the div (you don't necessarily need a child div) outside of the viewport. You can use the CSS units vh and vw to implement that.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:96
(Diff revision 2)
>          self.marionette.navigate(test_html)
>          overflow_x = self.marionette.find_element(By.ID, "assertMe2")
>          self.assertFalse(overflow_x.is_displayed())
>  
>      def testShouldShowElementNotVisibleWithHiddenAttribute(self):
> -        test_html = self.marionette.absolute_url("hidden.html")
> +        self.marionette.navigate(inline("""<div id='singleHidden' hidden>This will not be visible</div>"""))

Same as above. Try to reduce as much as possible. The div would even not need an id, you could use `find_element` with `By.TAG_NAME, "div"` instead.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:104
(Diff revision 2)
>  
>      def testShouldShowElementNotVisibleWhenParentElementHasHiddenAttribute(self):
> -        test_html = self.marionette.absolute_url("hidden.html")
> -        self.marionette.navigate(test_html)
> +        self.marionette.navigate(inline("""
> +        <div id='parent' hidden>
> +            <div id='child'>My parent is hidden so you can't see me</div>
> +        </div>"""))

You might want to use the same updated HTML content as from the very first changed test in this patch.
Attachment #8954484 - Flags: review?(hskupin) → review-
Hi Raymond, do you have an update for us? It would be great to get this patch finished and landed. Thanks.
Flags: needinfo?(paarthurnax)
Sadly we didn't receive any response from Raymond. Because of that I will put back this bug into the new state, and everyone else who want to continue to work on the patch is welcome to do so.
Assignee: paarthurnax → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(paarthurnax)
Attachment #8973180 - Flags: review?(hskupin)
Comment on attachment 8973180 [details] [diff] [review]
Removed hidden.html and changed its references to inline

These are the changes that I think are required according to the bug.
I also wanted to know who can push a commit to the try server for testing.
Attachment #8973180 - Flags: review?(hskupin) → review?(ato)
Attachment #8954484 - Attachment is obsolete: true
(In reply to Aditya Khadse from comment #29)

> These are the changes that I think are required according to theg.
> bu I also wanted to know who can push a commit to the try serverg.
> for testing.

I’ve applied for commit access level 1 for you in
https://bugzilla.mozilla.org/show_bug.cgi?id=1459346 which will
allow you to push changesets to try for testing.

PLease read
https://www.mozilla.org/en-US/about/governance/policies/commit/
carefully and complete the missing steps.
Assignee: nobody → akk5597
Status: NEW → ASSIGNED
Comment on attachment 8973180 [details] [diff] [review]
Removed hidden.html and changed its references to inline

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

Thanks again for a great patch!

There is a test failure due to div#singleHidden being renamed to
div#singlehidden that you need to address.

Whilst at it, can you also use double quotes consistently, since
this is the style the file is already written in?

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +117,1 @@
>          singleHidden = self.marionette.find_element(By.ID, 'singleHidden')

s/singleHidden/singlehidden/
Attachment #8973180 - Flags: review?(ato) → review-
Changed the inline to use """ instead of ''' and fixed the singleHidden id so that it can be selected
Attachment #8973379 - Flags: review?(ato)
Hm, so you appear to have posted a fixup to the first commit.  Any
chance you can squash them together so that the commit history on
central will have more meaning?

If you don’t have time for it I can do it.  Just let me know.
Attachment #8973379 - Flags: review?(ato) → review-
(In reply to Andreas Tolfsen ‹:ato› from comment #34)
> Hm, so you appear to have posted a fixup to the first commit.  Any
> chance you can squash them together so that the commit history on
> central will have more meaning?
> 
> If you don’t have time for it I can do it.  Just let me know.

I have the time to do it, but I do not understand how to do it.

If you tell me the steps I should take I can follow them.

I have worked with Git and there I usually don't commit until the very end, and just add files.

Also how can I undo my changes like git stash does, I have searched online, but couldn't find much.
Flags: needinfo?(ato)
I use git for committing to central so you would have to translate
these steps into the equivalent Mercurial steps, but I understand
the process is similar.  In hg I believe the rebase step is called
"histedit", which you may need a Mercurial extension for.

First you’d need to find the fork point, e.g. the commit on the
central branch where you branched off.  Imagining we have a branch
called "test" that was branched off "central" with some commits on
it:

> % git merge-base test central
> 2023a686862707651f11b5480489b87ba85d9cd8

You can also use "git log" to find the commit preceding your first.

We then use that commit as the basis for an interactive rebase (or
"histedit" in hg terminology):

> % git rebase -i 2023a686862707651f11b5480489b87ba85d9cd8

This will open up EDITOR showing us all the commits made:

> pick 123ae5f10d12 first commit
> pick bafed0f06917 second commit
>   
> # Rebase 2023a6868627..bafed0f06917 onto 2023a6868627 (1 command)
> #
> # Commands:
> # p, pick = use commit
> # r, reword = use commit, but edit the commit message
> # e, edit = use commit, but stop for amending
> # s, squash = use commit, but meld into previous commit
> # f, fixup = like "squash", but discard this commit's log message
> # x, exec = run command (the rest of the line) using shell
> # d, drop = remove commit

The commented section shows us which rebase commands we can use.
The default is to not make any modifications to the history by
simply ‘picking’ each commit as it is.  Since we want to merge (or
‘fold’ in hg terms) them together, we could mark the second commit
with "squash".  This will use the commit, but meld it into the
previous commit.

So I modify the list to look like this:

> pick 123ae5f10d12 first commit
> s bafed0f06917 second commit

Then save and quit my editor, git runs the commands in sequence,
and leaves us with one commit:

> % git log -2
> commit 123ae5f10d12ce510c9d7376f11fa1caea95e3f7 (HEAD -> test)
> Author: Andreas Tolfsen <ato@sny.no>
> Date:   Sun May 6 15:56:35 2018 +0100
> 
>     first commit
>     
>     MozReview-Commit-ID: Lg666mltYai
> 
> commit 2023a686862707651f11b5480489b87ba85d9cd8 (mozilla/central, central)
> Merge: 2f148c9fc61c 37a0f9e89fc7
> Author: Csoregi Natalia <ncsoregi@mozilla.com>
> Date:   Sat May 5 12:47:28 2018 +0300
> 
>     Merge inbound to mozilla-central.  a=merge
Flags: needinfo?(ato)
Instead of squasing, I removed my changes and updated my local files and redid the changes as it seemed easier. I have not been able to test the changes locally as running mach marionette-test path/to/file timed out after 120s saying it could not connect to Marionette server, same result when I tried mach test instead of mach marionette-test.
Attachment #8973180 - Attachment is obsolete: true
Attachment #8973379 - Attachment is obsolete: true
Attachment #8973544 - Flags: review?(ato)
(In reply to Aditya Khadse from comment #37)

> I have not been able to test the changes locally as running mach
> marionette-test path/to/file timed out after 120s saying it could
> not connect to Marionette server, same result when I tried mach
> test instead of mach marionette-test.

That is odd.  I tried "./mach test -z FILE" locally and can’t
reproduce this.  Do you possibly have some old Firefoxen running on
your system or have an outdated build?
(In reply to Andreas Tolfsen ‹:ato› from comment #38)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1378e63276f1fbba368829af6cc9bc6082aa582a

I understand why this particular push failed (I didn't add the hidden attribute to div#singleHidden). I will fix it ASAP and could have been avoided if I had run the tests locally, so my first step now is to get my local testing working.

In Comment 25, Henrik mentions that test_clicking_element_out_of_view_succeeds is incorrect since we are clicking an element that is hidden but not out of view, and says that we can use CSS vw and vh to move the div out of viewport. And the solution would be to replace that HTML with a div tag that has a style attribute as "position:relative; left:100vw; top:100vh;", as this would move the div out of viewport. Should I add this change too?
Flags: needinfo?(hskupin)
Flags: needinfo?(ato)
(In reply to Aditya Khadse from comment #40)
> avoided if I had run the tests locally, so my first step now is to get my
> local testing working.

If you still have problems with that then let us get this fixed. You can join us on irc.mozilla.org in the #ateam channel where we can directly work on that.

> In Comment 25, Henrik mentions that
> test_clicking_element_out_of_view_succeeds is incorrect since we are
> clicking an element that is hidden but not out of view, and says that we can
> use CSS vw and vh to move the div out of viewport. And the solution would be
> to replace that HTML with a div tag that has a style attribute as
> "position:relative; left:100vw; top:100vh;", as this would move the div out
> of viewport. Should I add this change too?

Yes please. Feel free to do it in the same changeset, given that the former test was simply wrong. But note to reduce the rate of intermittent failures don't use 100 for both, but a larger value like 200 to make sure it's definitely out of view.
Flags: needinfo?(hskupin)
Flags: needinfo?(ato)
This patch should be the last one; I have got local testing working again, I rebuilt after using mach clobber (which I think is needed after a major change, and needs a new build instead of build faster), I also changed test_clicking_element_out_of_view_succeeds
Attachment #8973605 - Flags: review?(hskupin)
Attachment #8973544 - Attachment is obsolete: true
Attachment #8973544 - Flags: review?(ato)
(In reply to Aditya Khadse from comment #42)
> This patch should be the last one; I have got local testing working again, I
> rebuilt after using mach clobber (which I think is needed after a major
> change, and needs a new build instead of build faster), I also changed

A clobber is not necessarily needed but you definitely have to run "mach build" after pulling. Otherwise there are inconsistencies especially when you are using artifact builds. Good to hear that you got it working. I will have a look at the patch later today. Thanks!
Comment on attachment 8973605 [details] [diff] [review]
Removed hidden.html and changed references to inline, fixed a test that needed div out of viewport

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

Code-wise this patch looks fine. But can you please fix the mentioned inline comments? I would appreciate that. Thanks.

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ +109,5 @@
> +        self.marionette.navigate(inline("""
> +        <div id='parent' hidden>
> +            <div id='child'>foo</div>
> +        </div>
> +        """))

Please indent the JS code by 4 chars similar to other tests. You can also get rid of the parent id, maybe even of the child id if you change the inner div to a p element. Then use `find_element(By.TAG_NAME, "p")` to retrieve the paragraph.

::: testing/marionette/harness/marionette_harness/tests/unit/test_legacy_mouse_action.py
@@ +44,5 @@
>          # The action based click doesn"t check for visibility.
> +        self.marionette.navigate(inline("""
> +        <div id='out_of_viewport' style="position:relative; left:200vw; top:200vh;">foo</div>
> +        """))
> +        el = self.marionette.find_element(By.ID, "out_of_viewport")

Similar to the other file. Indent the JS code and try to reduce the JS code. The id is not needed, same to the left style change.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +121,5 @@
> +        self.marionette.navigate(inline("""
> +        <div id='parent' hidden>
> +            <div id='child'>foo</div>
> +        </div>
> +        """))

For both changes please apply the changes as mentioned earlier.
Attachment #8973605 - Flags: review?(hskupin) → review-
I ran the tests locally and found no failures of the tests with edits by me (there were however 2 other failures), I changed some of the attributes as suggested and reduced the overall HTML code
Attachment #8973975 - Flags: review?(hskupin)
Attachment #8973605 - Attachment is obsolete: true
Thank you. I will have a look soon. Please note that for next bugs you want to work on we would appreciate review requests via mozreview. You can read about it here: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#mozreview-commits
Okay, I have gone through the provided link, and I should be able to use mozreview on my next bug. Thanks!
Comment on attachment 8973975 [details] [diff] [review]
Removed hidden.html and changed references to inline, reduced HTML code of related tests

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

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ +107,5 @@
>  
>      def test_clicking_an_element_that_is_not_displayed_raises(self):
> +        self.marionette.navigate(inline("""
> +            <p hidden>foo</p>
> +        """))

Now you have invalidated the test given that you removed the outer parent div element. Here the test clearly checks a child for visibility.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +120,5 @@
>      def testShouldShowElementNotVisibleWhenParentElementHasHiddenAttribute(self):
> +        self.marionette.navigate(inline("""
> +            <div id='parent' hidden>
> +                <div id='child'>foo</div>
> +            </div>

Make the child a paragraph and remove both ids.
Attachment #8973975 - Flags: review?(hskupin) → review-
Comment on attachment 8973975 [details] [diff] [review]
Removed hidden.html and changed references to inline, reduced HTML code of related tests

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

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ +107,5 @@
>  
>      def test_clicking_an_element_that_is_not_displayed_raises(self):
> +        self.marionette.navigate(inline("""
> +            <p hidden>foo</p>
> +        """))

Please drop that part. The test name matches with the updated test now, and was wrong before. Maybe next time add a comment and explain why you have made this change.
Changed the testShouldShowElementNotVisibleWhenParentElementHasHiddenAttribute as suggested in Comment 48
Attachment #8974058 - Flags: review?(hskupin)
Attachment #8973975 - Attachment is obsolete: true
Comment on attachment 8974058 [details] [diff] [review]
Removed hidden.html, changed references and fixed test_clicking_an_element_that_is_not_displayed_raises in test_click.py

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

Code changes look fine now. Please check the inline comment, and also update the commit message so it is better understandable, like:

> Bug 1434907 - [marionette] Remove dependency to hidden.html in unit tests.

r=me with those two changes. Thanks!

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py
@@ +105,5 @@
>          self.marionette.find_element(By.ID, "testDiv")
>          self.assertEqual(self.marionette.title, "Marionette Test")
>  
>      def test_clicking_an_element_that_is_not_displayed_raises(self):
> +        # This test checks if an element that is not displayed raises an error when clicked

No need for this extra comment. Please remove it.
Attachment #8974058 - Flags: review?(hskupin) → review+
Removed the comment and changed commit message as suggested in Comment 51
Attachment #8974302 - Flags: review?(hskupin)
Attachment #8974058 - Attachment is obsolete: true
Comment on attachment 8974302 [details] [diff] [review]
[marionette] Remove dependency to hidden.html in unit tests

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

Looks fine. I pushed a try build. If all tests pass we can get this patch landed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89b7452db3e6f8bf68ab720f9eb1e9e9bb56314a
Attachment #8974302 - Flags: review?(hskupin) → review+
Whiteboard: [lang=py] → [lang=py][checkin-needed]
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac582f3509d4
[marionette] Remove dependency to hidden.html in unit tests. r=whimboo
Keywords: checkin-needed
Whiteboard: [lang=py][checkin-needed] → [lang=py]
https://hg.mozilla.org/mozilla-central/rev/ac582f3509d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: