1441437 - Remove dependency to cssTransform.html and cssTransform2.html in test_visibility.py

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The following test is using external HTML testcases, which are simple enough to get integrated as data URL directly into the test. For Marionette unit tests we make use of the `inline` method. Here an example:

https://dxr.mozilla.org/mozilla-central/rev/a928be5dacc3b544e29c0612b3f8cda6447df802/testing/marionette/harness/marionette_harness/tests/unit/test_typing.py#14

The same method should be applied to the following test:

https://dxr.mozilla.org/mozilla-central/rev/default/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py

By adding as minimal necessary code from the following HTML testcase:

https://dxr.mozilla.org/mozilla-central/rev/default/testing/marionette/harness/marionette_harness/www/cssTransform.html
https://dxr.mozilla.org/mozilla-central/rev/default/testing/marionette/harness/marionette_harness/www/cssTransform2.html

The HTML testcase files should be deleted afterward.

Comment 1

[sabot]

Attachment: cssTransform.patch

See attached patch. Is it okay?

Comment 2

[sabot]

Comment on attachment 8955849 [details] [diff] [review]
cssTransform.patch

>diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py b/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
>--- a/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
>+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
>@@ -74,8 +74,39 @@ class TestVisibility(MarionetteTestCase)
>         self.assertFalse(shown.is_displayed())
> 
>     def testShouldSayElementsWithNegativeTransformAreNotDisplayed(self):
>-        test_html = self.marionette.absolute_url("cssTransform.html")
>-        self.marionette.navigate(test_html)
>+        cssTransform = """ <style>
>+        #parentY {
>+          transform: translateY(-10000px);
>+          -webkit-transform: translateY(-10000px);
>+          -o-transform: translateY(-10000px);
>+          -ms-transform: translateY(-10000px);
>+          -moz-transform: translateY(-10000px);
>+        }
>+        #parentX {
>+          transform: translateX(-10000px);
>+          -webkit-transform: translateX(-10000px);
>+          -o-transform: translateX(-10000px);
>+          -ms-transform: translateX(-10000px);
>+          -moz-transform: translateX(-10000px);
>+        }
>+        </style>
>+        <div id='zero-tranform'>
>+        You shouldn't see anything other than this sentence on the page
>+        </div>
>+        <div id='parentY'>
>+          I have a hidden child
>+          <div id='childY'>
>+            I am a hidden child
>+          </div>
>+        </div>
>+        <div id='parentX'>
>+          I have a hidden child
>+          <div id='childX'>
>+            I am a hidden child
>+          </div>
>+        </div>"""
>+        self.marionette.navigate(inline(cssTransform))
> 
>         elementX = self.marionette.find_element(By.ID, 'parentX')
>         self.assertFalse(elementX.is_displayed())
>@@ -83,8 +114,42 @@ class TestVisibility(MarionetteTestCase)
>         self.assertFalse(elementY.is_displayed())
> 
>     def testShouldSayElementsWithParentWithNegativeTransformAreNotDisplayed(self):
>-        test_html = self.marionette.absolute_url("cssTransform.html")
>-        self.marionette.navigate(test_html)
>+        cssTransform = """
>+        <style>
>+        #parentY {
>+          transform: translateY(-10000px);
>+          -webkit-transform: translateY(-10000px);
>+          -o-transform: translateY(-10000px);
>+          -ms-transform: translateY(-10000px);
>+          -moz-transform: translateY(-10000px);
>+        }
>+        #parentX {
>+          transform: translateX(-10000px);
>+          -webkit-transform: translateX(-10000px);
>+          -o-transform: translateX(-10000px);
>+          -ms-transform: translateX(-10000px);
>+          -moz-transform: translateX(-10000px);
>+        }
>+        </style>
>+        <div id='zero-tranform'>
>+        You shouldn't see anything other than this sentence on the page
>+        </div>
>+        <div id='parentY'>
>+          I have a hidden child
>+          <div id='childY'>
>+            I am a hidden child
>+          </div>
>+        </div>
>+        <div id='parentX'>
>+          I have a hidden child
>+          <div id='childX'>
>+            I am a hidden child
>+          </div>
>+        </div>
>+        """
>+        self.marionette.navigate(inline(cssTransform))
> 
>         elementX = self.marionette.find_element(By.ID, 'childX')
>         self.assertFalse(elementX.is_displayed())
>@@ -92,15 +157,57 @@ class TestVisibility(MarionetteTestCase)
>         self.assertFalse(elementY.is_displayed())
> 
>     def testShouldSayElementWithZeroTransformIsVisible(self):
>-        test_html = self.marionette.absolute_url("cssTransform.html")
>-        self.marionette.navigate(test_html)
>+        cssTransform = """
>+        <style>
>+        #zero-transform {
>+          transform: translateY(0px);
>+          -webkit-transform: translateY(0px);
>+          -o-transform: translateY(0px);
>+          -ms-transform: translateY(0px);
>+          -moz-transform: translateY(0px);
>+          transform: translateX(0px);
>+          -webkit-transform: translateX(0px);
>+          -o-transform: translateX(0px);
>+          -ms-transform: translateX(0px);
>+          -moz-transform: translateX(0px);
>+        }
>+        </style>
>+        <div id='zero-tranform'>
>+        You shouldn't see anything other than this sentence on the page
>+        </div>
>+        """
>+        self.marionette.navigate(inline(cssTransform))
> 
>         zero_tranform = self.marionette.find_element(By.ID, 'zero-tranform')
>         self.assertTrue(zero_tranform.is_displayed())
> 
>     def testShouldSayElementIsVisibleWhenItHasNegativeTransformButElementisntInANegativeSpace(self):
>-        test_html = self.marionette.absolute_url("cssTransform2.html")
>-        self.marionette.navigate(test_html)
>+        cssTransform2 = """
>+        <style>
>+        #negative-percentage-transformY{
>+          transform: translateY(-75px);
>+          -webkit-transform: translateY(-75%);
>+          -o-transform: translateY(-75%);
>+          -ms-transform: translateY(-75%);
>+          -moz-transform: translateY(-75%);
>+        }
>+        .block {
>+          display = block;
>+        }
>+        </style>
>+        <div class='block'>
>+          <br/>
>+        </div>
>+          <br/>
>+        <div class='block'>
>+        </div>
>+        <div id='negative-percentage-transformY'>I am not a hidden element </div>
>+        """
>+        self.marionette.navigate(inline(cssTransform2))
>         negative_percent__tranform = self.marionette.find_element(By.ID, 'negative-percentage-transformY')
>         self.assertTrue(negative_percent__tranform.is_displayed())
>

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8955849 [details] [diff] [review]
cssTransform.patch

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

Thank you for the patch, and see my review comments inline. For further updates it would be great if you could sent a mozreview request instead.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +74,4 @@
>          self.assertFalse(shown.is_displayed())
>  
>      def testShouldSayElementsWithNegativeTransformAreNotDisplayed(self):
> +        #test_html = self.marionette.absolute_url("cssTransform.html")

Don't forget to remove this line and the HTML testcase.

@@ +74,5 @@
>          self.assertFalse(shown.is_displayed())
>  
>      def testShouldSayElementsWithNegativeTransformAreNotDisplayed(self):
> +        #test_html = self.marionette.absolute_url("cssTransform.html")
> +        cssTransform = """ <style>

Nearly all of this code is duplicated. See how you can parametrize it, so it only needs to be defined once.

Comment 4

[sabot]

Hello, I am still a little confused on how it should be parametrized. Like should there be one string that contains all of cssTransform.html and each tests uses inline on that? Or something else?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

First you want to check which HTML content you have added in all the cases, and where it is different. Then you can move the string template into its own function and allow one or more parameters which get inserted into the template at the right places like `"""Hello {}""".format(parameter)`. So calling this method like `function("world")` would return `Hello world`.

Comment 6

[sabot]

Attachment: cssTransform1.patch

So I started by parametrizing but noticed there was still a lot of repetition in the code. My current patch seems to be a lot less redundant. How is this?

Also I was wondering what the benefit was to integrating all the tests into the python files rather than having them be separate. It seems like it might be more maintainable separating the HTML and CSS? And if down the road another python test file wants to use the same set of html on different tests it would require more redundant code rather than using a separate html file. I get that having the functions that generate the strings make it so that only one thing needs to be updated, but it seems less modular than before. I might be totally misunderstanding something but just wanted to mention my concern. Thank you!

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to sabot from comment #6)
Sorry for the late reply but I was away most of last week.

> So I started by parametrizing but noticed there was still a lot of
> repetition in the code. My current patch seems to be a lot less redundant.
> How is this?

> Also I was wondering what the benefit was to integrating all the tests into
> the python files rather than having them be separate. It seems like it might
> be more maintainable separating the HTML and CSS? And if down the road

Having them integrated makes it easier for managing those resources. The mess we currently have is that different tests make use of the same HTML file, and as such you have to be very careful in changing the content to not affect other tests, or make them invalid. As such each test should really have its own HTML testcase.

> another python test file wants to use the same set of html on different
> tests it would require more redundant code rather than using a separate html
> file. I get that having the functions that generate the strings make it so
> that only one thing needs to be updated, but it seems less modular than
> before. I might be totally misunderstanding something but just wanted to
> mention my concern. Thank you!

I only filed bugs so far for those files which are only used in a single or two tests. What we did for other bugs like that one is to strip down the HTML content to only those necessary parts absolutely required for the test method. Everything else should be removed. Then if the template looks closely the same parameters can be added as you did in the last update. 

I share you concern that in some cases it will not be possible. So let me have a look at the patch again, so we can figure out if we indeed have to get rid of some of those changes.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh, and regarding the patch, we love to see patches being uploaded to mozreview instead of being attached directly to the bug. Please see the link in the user story of this bug and follow the appropriate sections.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8957407 [details] [diff] [review]
cssTransform1.patch

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

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +34,5 @@
> +      transform: translateY(-10000px);
> +      -webkit-transform: translateY(-10000px);
> +      -o-transform: translateY(-10000px);
> +      -ms-transform: translateY(-10000px);
> +      -moz-transform: translateY(-10000px);

Given that our tests are run to test web standards we should probably omit the old browser specific prefixes, and only keep the property as defined in the CSS spec.

Note that those HTML files have been created ages ago and would need an update for the content anyway.

@@ +65,5 @@
> +      <div id='childY'>
> +        I am a hidden child
> +      </div>
> +    </div>
> +    <div id='parentX'>

The whole `parentX` div is identical to `parentY` except that the translation is different. Means we could rip this out, and add a variable for the rotation direction.

@@ +120,4 @@
>          self.assertFalse(shown.is_displayed())
>  
>      def testShouldSayElementsWithNegativeTransformAreNotDisplayed(self):
> +        self.marionette.navigate(css_Transform())

The line we would need here could look like:

>  self.marionette.navigate(inline("""<div style="transform: translateX(-10000px)"></div""">)

@@ +159,5 @@
> +          <br/>
> +        </div>
> +          <br/>
> +        <div class='block'>
> +        </div>

Try to use some new and modern CSS definitions to replace those blocks, which probably only exists to push down the `negative-percentage-transformY` element a bit.

Comment 10

[sabot]

Hello,

Sorry about the slow response. I am a student heading into finals so I've got a bunch of projects due and then finals to study for. I am not sure if I will have time to work on this in the near future but I will try.
Thank you!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

sabot, would you feel better if we assign this bug to someone else then? Thank you for digging into so far.

Comment 12

[sabot]

Attachment: cssTransform2.patch

Hello, I had a few minutes to play around with this. Hopefully this is slightly better. Sorry for not using the MozReview tool thing, I just don't have a ton of time to figure out how to use it.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sabot, I have to say sorry that this review request slipped through. Looks like it somewhat happened because I had to set it to myself. I will check my mail filters. Best will be in the future when you set it yourself once a new version of the patch is ready. Also feel free to ping me at any time in case of a situation like that, or when you are stuck. As mentor I'm here to help.

Given that I'm at a training this week I will try to do the review as soon as I can.

Comment 14

[sabot]

Great thank you so you so much!

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8964599 [details] [diff] [review]
cssTransform2.patch

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

Thank you for the update of the patch. And sorry again that it has been taken a while.

I checked the code and made some comments for blocks which seem to be unclear. I hope the examples I added will help you to understand in how the code can be improved. If there are questions please let me know here on Bugzilla, or join us on irc.mozilla.org/#ateam.

::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
@@ +36,5 @@
> +      -webkit-transform: translateX(-10000px);
> +      -o-transform: translateX(-10000px);
> +      -ms-transform: translateX(-10000px);
> +      -moz-transform: translateX(-10000px);
> +    }

I would propose that you just create a method which only returns the CSS styles. You can do that like that:

```
get_css_transformation(direction, offset):
   return %string_format%.format(direction.upper(), offset)
```

The method should as best return the above full block of properties, unless you know that one of those is not needed anymore and we can completely rely on the `transform` property.

@@ +46,5 @@
> +      I have a hidden child
> +      <div id='childX'>
> +        I am a hidden child
> +      </div>
> +    </div>"""

The HTML content we might not need here but it can be used with inline in the specific tests. Note, that currently some transformations are not applied correctly, eg. for `childX`.

@@ -79,5 @@
>  
>          elementX = self.marionette.find_element(By.ID, 'parentX')
>          self.assertFalse(elementX.is_displayed())
> -        elementY = self.marionette.find_element(By.ID, 'parentY')
> -        self.assertFalse(elementY.is_displayed())

Here you removed the checks for `translateY`, which we want to keep. You can do the test like:

```
    for direction in ['X', 'Y']:
        url = inline(%format%.format(get_css_transformation(direction, "-10000px"))
```

@@ -88,5 @@
>  
>          elementX = self.marionette.find_element(By.ID, 'childX')
>          self.assertFalse(elementX.is_displayed())
> -        elementY = self.marionette.find_element(By.ID, 'childY')
> -        self.assertFalse(elementY.is_displayed())

Same here.

@@ +109,5 @@
>  
>      def testShouldSayElementWithZeroTransformIsVisible(self):
> +        self.marionette.navigate(inline("""<div id='zero-tranform'>
> +        You shouldn't see anything other than this sentence on the page
> +        </div>"""))

Feel free to remove the id of the element and shorten the text to something like "foo". The element you can then retrieve via `By.TAG_NAME` and "div".

@@ +125,5 @@
> +          -ms-transform: translateY(-75%);
> +          -moz-transform: translateY(-75%);
> +        }
> +        </style>
> +        <div id='negative-percentage-transformY'>I am not a hidden element </div>

You can remove the id and directly apply the style to the `div` class. As best also use the above mentioned `get_css_transformation()` method.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8964599 [details] [diff] [review]
cssTransform2.patch

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

Oh, and please remember to also remove the two not used HTML testcase files.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sabot, do you have an update for us?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No update from Sabot. If anyone wants to work on the bug please feel to comment. Thanks.

Comment 19

[shivambalikondwar]

i am an undergrad in engineering , looking forward to help open-source orgs,i have goals to help mozilla through gsoc 
can i solve it ,i have tried to write some python code (web-scrapers) so i am quite good with regex and file handling and as i opened the file it seems easy  lately
so :)

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi, you are welcome to help on this bug. When you can provide patch the bug will be assigned to you. Please read the user story in how to get started.

Comment 21

[shivambalikondwar]

ok thanks i am on my work

Comment 22

[shivambalikondwar]

i have ready but how i submit or first get reviewed?

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the patch you have you need to upload to phabricator for getting reviewed by myself. Please see https://moz-conduit.readthedocs.io/ in how to setting it up.

Comment 24

[shivambalikondwar]

Attachment: Bug 1441437 - test_visibility.py changed

Comment 25

[shivambalikondwar]

pardon me if i made any noobie mistakes

Comment 26

[shivambalikondwar]

i got it and i am working on the changes that you suggested, i will start from last patch :)

Comment 27

[shivambalikondwar]

Attachment: Bug 1441437 - upgrade

Depends on D6877

Comment 28

[shivambalikondwar]

Attachment: Bug 1441437 - done1

Comment 29

[shivambalikondwar]

@reporter the patch is submitted kindly review it

Comment 30

[shivambalikondwar]

Attachment: Bug 1441437 - done2

Depends on D8081

Comment 31

[shivambalikondwar]

Attachment: Bug 1441437 - done1

Depends on D8081

Comment 32

[shivambalikondwar]

Attachment: Bug 1441437 - done1

Depends on D9001

Comment 33

[shivambalikondwar]

Attachment: Bug 1441437 - this is the most current patch

Comment 34

[shivambalikondwar]

Attachment: Bug 1441437 - recent one

Comment 35

[shivambalikondwar]

Attachment: Bug 1441437 - changed

Comment 37

[shivambalikondwar]

Attachment: Bug 1441437 - just for sample

Depends on D10997

Comment 38

[shivambalikondwar]

Attachment: Bug 1441437 html_code added

Depends on D10997

Comment 39

[shivambalikondwar]

Attachment: Bug 1441437 cssTransform and cssTransform2 integrated in test_visibility.py

I fixed the bug now it runs fine
  the optimizations are enough , they don't introduce any unnecesary
  code
  i checked the code using marionette test and it ran fine
  i am waiting on your approval

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry for the late reply but as you have known I was away mostly all the last month. I'm digging through my bug mail and getting close to this review.

One thing I want to note again which I feel is very important, and hasn't still be obeyed after discussing it several times. Whenever you push an update a new phabricator revision gets created, which completely kills my review process. It means all previous comments are simply lost.

As such I will review the current state again, but once you are going to create another new revision, and don't re-use the current one, I will unassign you from the bug. I feel sorry about but it actually causes me a lot more work as expected. So please obey the documentation about patch submission, and if unclear ask before-hand. Thanks.

Comment 41

[shivambalikondwar]

Ok,I have caused a lot of problems submitting my patch. I didn’t want to waste your valuable time in repetitive tasks as such.
So here is the thing,
I have given it a considerable amount of time, tested it and i want to complete it.If I create a new revision this time,I’ll back off and let someone else work on it.
Rest all depends on you

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that we weren't successful in fixing this bug for a while through different people I'm going to remove it from the mentored list. Maybe I find time soon to get it fixed myself.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I took a look and checked what we can do. Here the results:

• The CSS declarations in the HTML testcase files still use vendor prefixes. Given that our tests only run with Firefox we don't need those for other browsers, but even not the -moz-transform one for Firefox because we fully support the CSS specification since Firefox 16.

• I reduced the HTML code and noticed that the tests can be kinda self-contained. There is actually no need to have a helper to create the CSS declarations, because those are that minimal.

Here a try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e852f315912007e01ddc1b34ac0aa11fc8e5ed37

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1441437 - [marionette] Remove dependency to cssTransform.html and cssTransform2.html in test_visibility.py. r?ato

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e15fb11a5c6d
[marionette] Remove dependency to cssTransform.html and cssTransform2.html in test_visibility.py. r=ato

Comment 46

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e15fb11a5c6d