1434906 - Remove dependency to datetimePage.html in test_date_time_value.py

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

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

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/default/testing/marionette/harness/marionette_harness/tests/unit/test_date_time_value.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/datetimePage.html

The HTML testcase should be deleted afterward.

Comment 1

[Daniel Marshall (:starsandspirals)]

Is it okay if I submit a patch for this? This is the first bug I've worked on!

Comment 2

[Daniel Marshall (:starsandspirals)]

Attachment: Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py

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

Comment 3

[Daniel Marshall (:starsandspirals)]

Attachment: Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py;

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

Comment 4

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

(In reply to Daniel Marshall (:starsandspirals) from comment #1)
> Is it okay if I submit a patch for this? This is the first bug I've worked
> on!

Surely! It's great to see that you were even able to attach a patch via mozreview without any help. Good work! Once it is ready please make sure to also flag r? from me. You can do that when clicking review on one of your attachments. Thanks!

Comment 5

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

Comment on attachment 8948813 [details]
Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py

https://reviewboard.mozilla.org/r/218184/#review224952

Did you run the test to verify that it works? As what I can see it fails. Please run the test and verify yourself what's missing.

::: testing/marionette/harness/marionette_harness/tests/unit/test_date_time_value.py:20
(Diff revision 1)
> +    return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))
> +
> +
>  class TestDateTime(MarionetteTestCase):
>      def test_set_date(self):
> -        test_html = self.marionette.absolute_url("datetimePage.html")
> +        self.marionette.navigate(inline("<input id=\"date-test\" type=\"date\"/>")

You can use single quotes inside the string to not have to use the escape characters.

Comment 6

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

Comment on attachment 8948814 [details]
Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py;

https://reviewboard.mozilla.org/r/218186/#review224956

Please combine this commit with the other one by using `hg histedit`. Thanks.

Comment 7

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8948814 [details]
Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py;

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

Comment 8

[Daniel Marshall (:starsandspirals)]

I fixed the missing import statement, replaced the escape characters with single quotes, and combined the commits. Thanks for the feedback!

Comment 9

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

Comment on attachment 8948814 [details]
Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py;

https://reviewboard.mozilla.org/r/218186/#review225590

This patch now misses the removal of the HTML testcase file. Maybe it got lost during a rebase?

Comment 10

[Daniel Marshall (:starsandspirals)]

The patch seems to include the removal of the HTML file to me; I can't find the file any more, and when I look at the commit on reviewboard.mozilla.org, it says that datetimePage.html has been deleted. Am I making a silly mistake?

Comment 11

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

It's strange. When I checked it this morning I haven't seen it. Now it is present. Sorry, so that seems to have been my own fault. 

The patch is indeed fine! Thanks a lot.

Comment 12

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

Comment on attachment 8948814 [details]
Bug 1434906 - Removed dependency to datetimePage.html in test_date_time_value.py;

https://reviewboard.mozilla.org/r/218186/#review225756

Comment 13

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

Additionally to the review I triggered a try build via mozreview. If results are all green I will get it landed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/659180649c65
Removed dependency to datetimePage.html in test_date_time_value.py; r=whimboo

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/659180649c65

Comment 16

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

Daniel, if you are interested to work on a more challenging bug please let me know.

Comment 17

[Daniel Marshall (:starsandspirals)]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Daniel, if you are interested to work on a more challenging bug please let
> me know.

Yes, I would definitely be interested in working on something else!