1423661 - Remove dependency to blob_download.html in test_window_handles_content.py (test_window_handles_after_opening_new_non_browser_window)

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

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

The following test is using an external HTML testcase, which is 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/a928be5dacc3b544e29c0612b3f8cda6447df802/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py#100

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

https://dxr.mozilla.org/mozilla-central/rev/a928be5dacc3b544e29c0612b3f8cda6447df802/testing/marionette/harness/marionette_harness/www/blob_download.html

The HTML testcase should be deleted afterward.

Comment 1

[Vedant Chakravadhanula(:vedantc98)]

Attachment: Bug 1423661 - Added data url to replace blob-download.html.

Review commit: https://reviewboard.mozilla.org/r/206840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206840/

Comment 2

[Vedant Chakravadhanula(:vedantc98)]

Hi Henrik, I've added a patch with the changes.
Please let me know if there's any way I could reduce the html further, thanks.

Comment 3

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

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

https://reviewboard.mozilla.org/r/206840/#review213382

Please run the tests when you have made the changes. Right now that will fail because of missing imports. Further also run the linter to make sure the style is correct. Do that via `mach lint testing/marionette`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:106
(Diff revision 1)
>  
>      @skip_if_mobile("Fennec doesn't support other chrome windows")
>      def test_window_handles_after_opening_new_non_browser_window(self):
>          def open_with_link():
> -            self.marionette.navigate(self.marionette.absolute_url("blob_download.html"))
> +            self.marionette.navigate(inline("""
> +              <a id="blob-download" download="foo.html"> </a>

Please format the code so it is better readable and uses the correct indentation (as before in the HTML testcase).

Comment 4

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206840/diff/1-2/

Comment 5

[Vedant Chakravadhanula(:vedantc98)]

I've changed the formatting so it uses the right indentation.
I ran the tests and as you said, they fail because of missing imports. 
I ran the linter, which came back clean (0 problems).

Comment 6

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

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

https://reviewboard.mozilla.org/r/206840/#review215372

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:13
(Diff revision 2)
>  
>  from marionette_harness import MarionetteTestCase, skip_if_mobile, WindowManagerMixin
>  
>  
> +def inline(doc):
> +    return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc)) 

Please remove the trailing space, and add two empty lines before the class. This should have failed the linter task.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:112
(Diff revision 2)
> +            <script>
> +                const string = "test";
> +                const blob= new Blob([string], {type : "text/html"});
> +                const link=Document.getElementById("blob-download");
> +                link.href=URL.createObjectURL(blob);
> +            </script>

Please copy this block of text again from the original file. The above lines contain a lot of changes which we don't want, like white-spaces, missing link text, intendation...

Comment 7

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

Sorry for the late review, but I was away for a while. Let me know if you have questions regarding my just done review.

Comment 8

[David Burns :automatedtester]

[Mass Change 2018-01-15] Moving bugs to backlog

Comment 9

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

Hi Vedant, I just wanted to check back if you have an update for us?

Comment 10

[Vedant Chakravadhanula(:vedantc98)]

Hi Henrik, sorry I wasn't able to submit a patch for a while. I'll get on this now, should be done soon.

Comment 11

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206840/diff/2-3/

Comment 12

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

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

https://reviewboard.mozilla.org/r/206840/#review219252

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:13
(Diff revision 3)
>  
>  from marionette_harness import MarionetteTestCase, skip_if_mobile, WindowManagerMixin
>  
>  
> +def inline(doc):
> +    return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))

Please also run the test when you make changes. This fails because `urllib` isn't getting imported anywhere.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:107
(Diff revision 3)
>  
>      @skip_if_mobile("Fennec doesn't support other chrome windows")
>      def test_window_handles_after_opening_new_non_browser_window(self):
>          def open_with_link():
> -            self.marionette.navigate(self.marionette.absolute_url("blob_download.html"))
> +            self.marionette.navigate(inline("""
> +            <!DOCTYPE html>

We don't need that line here. Please also intend the whole JS code block by two chars.

Comment 13

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206840/diff/3-4/

Comment 14

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

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

https://reviewboard.mozilla.org/r/206840/#review221194

Looks fine. Can you please fix the nit? In the meantime I will trigger a try build. Thanks!

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:8
(Diff revision 4)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import absolute_import
>  
> +import urllib
>  import types

nit: please flip the order so it is alphabetical.

Comment 15

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206840/diff/4-5/

Comment 16

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

Comment on attachment 8935992 [details]
Bug 1423661 - Added data url to replace blob-download.html.

https://reviewboard.mozilla.org/r/206840/#review221194

> nit: please flip the order so it is alphabetical.

Thank you. That looks good now. Also try is green, so I'm going to land your patch now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/528e9f3cd1ed
Added data url to replace blob-download.html. r=whimboo

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/528e9f3cd1ed