Closed Bug 1441437 Opened 2 years ago Closed 8 months ago

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

Categories

(Testing :: Marionette, enhancement, P2)

Version 3
enhancement

Tracking

(firefox66 fixed)

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

(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, 15 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
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.
Attached patch cssTransform.patch (obsolete) — Splinter Review
See attached patch. Is it okay?
Attachment #8955849 - Flags: review?(hskupin)
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 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.
Attachment #8955849 - Flags: review?(hskupin) → review-
Assignee: nobody → sabot
Status: NEW → ASSIGNED
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?
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`.
Attached patch cssTransform1.patch (obsolete) — Splinter Review
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!
Attachment #8957407 - Flags: review+
Attachment #8957407 - Flags: review+
Attachment #8957407 - Flags: review?(hskupin)
(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.
Attachment #8955849 - Attachment is obsolete: true
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 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.
Attachment #8957407 - Flags: review?(hskupin)
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!
sabot, would you feel better if we assign this bug to someone else then? Thank you for digging into so far.
Flags: needinfo?(sabot)
Attached patch cssTransform2.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(sabot)
Attachment #8964599 - Flags: review?
Attachment #8964599 - Flags: review? → review?(hskupin)
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.
Great thank you so you so much!
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.
Attachment #8964599 - Flags: review?(hskupin) → review-
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.
Attachment #8957407 - Attachment is obsolete: true
Sabot, do you have an update for us?
Flags: needinfo?(sabot)
No update from Sabot. If anyone wants to work on the bug please feel to comment. Thanks.
Assignee: sabot → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(sabot)
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 :)
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.
ok thanks i am on my work
i have ready but how i submit or first get reviewed?
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.
pardon me if i made any noobie mistakes
Attachment #9012042 - Attachment description: Bug 1441437 - done → Bug 1441437 - test_visibility.py changed
i got it and i am working on the changes that you suggested, i will start from last patch :)
Attached file Bug 1441437 - upgrade (obsolete) —
Depends on D6877
Attached file Bug 1441437 - done1 (obsolete) —
@reporter the patch is submitted kindly review it
Assignee: nobody → shivambalikondwar
Attached file Bug 1441437 - done2 (obsolete) —
Depends on D8081
Attached file Bug 1441437 - done1 (obsolete) —
Depends on D8081
Attached file Bug 1441437 - done1 (obsolete) —
Depends on D9001
Attachment #9021066 - Attachment description: Bug 1441437 - this is the most current build → Bug 1441437 - this is the most current patch
Attachment #9017941 - Attachment is obsolete: true
Attachment #9012042 - Attachment is obsolete: true
Attachment #9017939 - Attachment is obsolete: true
Attachment #9017410 - Attachment is obsolete: true
Attachment #9015512 - Attachment is obsolete: true
Attachment #9015104 - Attachment is obsolete: true
Attached file Bug 1441437 - recent one (obsolete) —
Attachment #9021066 - Attachment is obsolete: true
Attached file Bug 1441437 - changed (obsolete) —
Attachment #9025011 - Attachment is obsolete: true
Duplicate of this bug: 1508999
Attached file Bug 1441437 - just for sample (obsolete) —
Depends on D10997
Attached file Bug 1441437 html_code added (obsolete) —
Depends on D10997
Attachment #9028036 - Attachment is obsolete: true
Attachment #9022839 - Attachment is obsolete: true
Attachment #9022839 - Attachment is obsolete: false
Attachment #9022839 - Attachment is obsolete: true
Attachment #9022839 - Attachment is obsolete: false
Attachment #9028042 - Attachment is obsolete: true
Attachment #9022839 - Attachment is obsolete: true
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
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.
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

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.

Assignee: shivambalikondwar → nobody
Mentor: hskupin
Keywords: good-first-bug

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

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P2
Attachment #9029593 - Attachment is obsolete: true
Attachment #8964599 - Attachment is obsolete: true
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e15fb11a5c6d
[marionette] Remove dependency to cssTransform.html and cssTransform2.html in test_visibility.py. r=ato
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.