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

RESOLVED FIXED in Firefox -esr52

Status

Testing
Marionette
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: whimboo, Assigned: Nitish, Mentored, NeedInfo)

Tracking

({good-first-bug})

Version 3
mozilla55
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [lang=py])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
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))
(Reporter)

Comment 1

9 months ago
Steps for getting started with contributing to the Mozilla code base can be found here:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors
Mentor: hskupin@gmail.com
Whiteboard: [lang=py]
(Reporter)

Updated

9 months ago
Blocks: 1336022
Keywords: good-first-bug

Comment 2

9 months ago
Hey Henrik, Can I work on this one?
Flags: needinfo?(hskupin)
(Reporter)

Comment 3

9 months ago
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.
Flags: needinfo?(hskupin) → needinfo?(nitishplus98)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8844114 - Flags: review?(hskupin)
(Reporter)

Comment 5

9 months ago
mozreview-review
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.
Attachment #8844114 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 7

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

Changes Made!
Attachment #8844114 - Flags: review?(hskupin)
(Reporter)

Comment 8

9 months ago
mozreview-review
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.
Attachment #8844114 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Flags: needinfo?(hskupin)
(Reporter)

Comment 10

9 months ago
mozreview-review
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.
Attachment #8844114 - Flags: review?(hskupin) → review+

Comment 11

9 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0456f9d126c
Wait for the web page being loaded in a newly opened tab r=whimboo

Comment 12

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0456f9d126c
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

9 months ago
Assignee: nobody → nitishplus98
Flags: needinfo?(hskupin)
(Assignee)

Comment 13

9 months ago
Thank you sir.
(Reporter)

Comment 14

9 months ago
This is a test-only fix which improves stability of tests. It would be great to get this uplifted to aurora and beta. Thanks.
status-firefox53: --- → affected
status-firefox54: --- → affected
Whiteboard: [lang=py] → [lang=py][checkin-needed-aurora][checkin-needed-beta]

Comment 15

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/65e1e4c74f9b
status-firefox54: affected → fixed
Whiteboard: [lang=py][checkin-needed-aurora][checkin-needed-beta] → [lang=py][checkin-needed-beta]

Comment 16

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/703f3f69fb43
status-firefox53: affected → fixed
(Reporter)

Comment 17

9 months ago
Actually it would be also good to see it on the esr52 branch.
Whiteboard: [lang=py][checkin-needed-beta] → [lang=py][checkin-needed-esr52]
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!
status-firefox53: fixed → affected
Flags: needinfo?(hskupin)
(Reporter)

Comment 19

9 months ago
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.
Flags: needinfo?(hskupin)
Whiteboard: [lang=py][checkin-needed-esr52] → [lang=py][needs patch on bug 1336022 uplifted first][checkin-needed-beta][checkin-needed-esr52]

Comment 20

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5715f53e02fd
status-firefox53: affected → fixed
(Reporter)

Updated

9 months ago
Whiteboard: [lang=py][needs patch on bug 1336022 uplifted first][checkin-needed-beta][checkin-needed-esr52] → [lang=py][needs patch on bug 1336022 uplifted first][checkin-needed-esr52]

Comment 21

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/c205140095b7
status-firefox-esr52: --- → fixed
Whiteboard: [lang=py][needs patch on bug 1336022 uplifted first][checkin-needed-esr52] → [lang=py]
You need to log in before you can comment on or make changes to this bug.