1344647 - Marionette unit tests have to use Wait().until() instead of assert* for calls to get_url()

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

There are similar test failures lately which seem to cover the same underlying issue. It's all about calling get_url() after opening, or closing tabs or windows.

Given that tests are running that fast there can be conditions that the expected URL hasn't been fully loaded yet and as such "about:blank" is getting reported.

We should fix this by refactoring all those calls to use Wait().until():

> self.assertEqual(self.marionette.get_url(), self.empty_page)

to

> Wait(self.marionette, self.marionette.timeout.page_load).until(
>     lambda _: self.marionette.get_url() == self.empty_page,
>     message="The expected page '{}' has not been loaded".format(self.empty_page))

Comment 1

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

Steps for getting started with contributing to the Mozilla code base can be found here:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

Comment 2

[Anjul Tyagi]

Hey Henrik, Can I work on this one?

Comment 3

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

Nitish already asked over on bug 1343039 to work on this. Lets wait for his reply please, or we can find a different bug for you to work on immediately. Just join on IRC and we can figure that out.

Comment 4

[Nitish]

Attachment: Bug 1344647 - Wait for the web page being loaded in a newly opened tab

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

Comment 5

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

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

https://reviewboard.mozilla.org/r/117650/#review119584

Hi Nitish, you didn't set the review request in mozreview but in Bugzilla. Please always do it in mozreview in the future so that it is getting synced correctly.

The code changes look fine, but the patch still needs some smaller updates.

::: commit-message-7b218:1
(Diff revision 1)
> +Bug 1344647 - use Wait().until() instead of assert* for calls to get_url in Marionette unit tests

Lets fix the message so it says: Bug 1344647 - Wait for the web page being loaded in a newly opened tab.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:37
(Diff revision 1)
>  
>          self.marionette.switch_to_window(new_tab)
>          self.assertEqual(self.marionette.current_window_handle, new_tab)
> -        self.assertEqual(self.marionette.get_url(), self.empty_page)
> +        Wait(self.marionette, self.marionette.timeout.page_load).until(
> +        lambda _: self.marionette.get_url() == self.empty_page,
> +        message="The expected page '{}' has not been loaded".format(self.empty_page))

code-wise this looks fine but please obey the indentation of the code inside of until(). Currently this will give linting failures.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:65
(Diff revision 1)
>          # Check that the new tab has the correct page loaded
>          self.marionette.switch_to_window(new_tab)
>          self.assertEqual(self.marionette.current_window_handle, new_tab)
> -        self.assertEqual(self.marionette.get_url(), self.empty_page)
> +        Wait(self.marionette, self.marionette.timeout.page_load).until(
> +        lambda _: self.marionette.get_url() == self.empty_page,
> +        message="The expected page '{}' has not been loaded".format(self.empty_page))

Same as above for indentation.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:91
(Diff revision 1)
>  
>          self.marionette.switch_to_window(new_tab)
>          self.assertEqual(self.marionette.current_window_handle, new_tab)
> -        self.assertEqual(self.marionette.get_url(), self.empty_page)
> +        Wait(self.marionette, self.marionette.timeout.page_load).until(
> +        lambda _: self.marionette.get_url() == self.empty_page,
> +        message="The expected page '{}' has not been loaded".format(self.empty_page))

Same as above for indentation.

Comment 6

[Nitish]

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

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

Comment 7

[Nitish]

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

Changes Made!

Comment 8

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

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

https://reviewboard.mozilla.org/r/117650/#review120510

Sorry but with the latest update the commit seems to be broken because it misses your first changes. The patch as is right now will not apply anymore. Please make sure that the commit contains all the changes you did for this bug. Thanks.

Comment 9

[Nitish]

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

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

Comment 10

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

Comment on attachment 8844114 [details]
Bug 1344647 - Wait for the web page being loaded in a newly opened tab

https://reviewboard.mozilla.org/r/117650/#review123084

Look fine. I will go ahead and land it for you.

Thank you a lot for your contribution which made this test more stable.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0456f9d126c
Wait for the web page being loaded in a newly opened tab r=whimboo

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Nitish]

Thank you sir.

Comment 14

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

This is a test-only fix which improves stability of tests. It would be great to get this uplifted to aurora and beta. Thanks.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65e1e4c74f9b

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/703f3f69fb43

Comment 17

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

Actually it would be also good to see it on the esr52 branch.

Comment 18

[Carsten Book [:Tomcat]]

backed this out seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=85047968&repo=mozilla-beta 

Henrik, could you take a look, thanks!

Comment 19

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

The problem here is that we miss the patch on bug 1336022 for beta and esr52. Please make sure to uplift the other patch first. Thanks.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5715f53e02fd

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c205140095b7