Intermittent test_window_handles_content.py TestWindowHandles.test_window_handles_after_opening_new_tab | AssertionError: u'about:blank' != 'http://127.0.0.1:36741/empty.html'

RESOLVED FIXED in Firefox -esr52

Status

Testing
Marionette
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: Treeherder Bug Filer, Assigned: Anjul Tyagi, Mentored)

Tracking

({intermittent-failure})

Version 3
mozilla54
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [lang=py])

MozReview Requests

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 months ago
treeherder
Filed by: cbook [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=73795699&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/aUAWhVM_TJGlaqdQ4kFVsQ/runs/0/artifacts/public/logs/live_backing.log
[task 2017-02-02T04:17:03.255931Z] 04:17:03     INFO - TEST-UNEXPECTED-FAIL | test_window_handles_content.py TestWindowHandles.test_window_handles_after_opening_new_tab | AssertionError: u'about:blank' != 'http://127.0.0.1:36741/empty.html'
[task 2017-02-02T04:17:03.256740Z] 04:17:03     INFO - Traceback (most recent call last):
[task 2017-02-02T04:17:03.256897Z] 04:17:03     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 166, in run
[task 2017-02-02T04:17:03.257486Z] 04:17:03     INFO -     testMethod()
[task 2017-02-02T04:17:03.258028Z] 04:17:03     INFO -   File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py", line 35, in test_window_handles_after_opening_new_tab
[task 2017-02-02T04:17:03.258133Z] 04:17:03     INFO -     self.assertEqual(self.marionette.get_url(), self.empty_page)

So when a new tab gets opened we automatically open about:blank first, before the target page gets loaded. See bug 610357 for details. This looks like a race condition and we should better make use of Wait().until().

Updated

10 months ago
Mentor: hskupin@gmail.com
Whiteboard: [lang=py]
(Assignee)

Comment 2

10 months ago
As per your suggestion for the bug #1334996, Can I work on this one ?
Flags: needinfo?(hskupin)
Yes, feel tree to start working on it. Similar links as given on the other bug will also apply here. Let me know if there is still something not clear. Thanks!
Flags: needinfo?(hskupin)
(Assignee)

Comment 4

10 months ago
Created attachment 8833982 [details] [diff] [review]
Expected solution to the race condition.
Attachment #8833982 - Flags: review?(hskupin)
Comment on attachment 8833982 [details] [diff] [review]
Expected solution to the race condition.

Those days we usually use mozreview (https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview) for review requests, but for the first patch the old style review will be fine.

> from marionette_harness import MarionetteTestCase, WindowManagerMixin
> 
>-
> class TestWindowHandles(WindowManagerMixin, MarionetteTestCase):

Please do not remove this empty line because it will cause a linter failure. The two empty lines are actually expected.

>         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)
>-
>+        # self.assertEqual(self.marionette.get_url(), self.empty_page)

There is no need to keep the old line commented out. Just remove it.

>+        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
>+            lambda mn: self.empty_page in mn.get_url())

This looks fine, but please fix the white-space issues. There should be no empty line before Wait().until() but afterward, just to separate the block of checks from the next action.

With the above changes you will get my r+. Btw did you run the tests locally to check that all is working fine?
Attachment #8833982 - Flags: review?(hskupin)
(Assignee)

Comment 6

9 months ago
Created attachment 8834351 [details] [diff] [review]
Patch v1.2 with spaces issue corrected.

Yes, I ran the marionette tests on my machine for this test file and they passed.
This is the command that I used:
./mach marionette-test testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
Attachment #8833982 - Attachment is obsolete: true
Attachment #8834351 - Flags: review?(hskupin)
Comment on attachment 8834351 [details] [diff] [review]
Patch v1.2 with spaces issue corrected.

Review of attachment 8834351 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py
@@ +32,5 @@
>  
>          self.marionette.switch_to_window(new_tab)
>          self.assertEqual(self.marionette.current_window_handle, new_tab)
> +        Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: self.empty_page in mn.get_url())

Maybe I didn't spot this last time, but this check should actually be a `==`, and not using `in`.
Attachment #8834351 - Flags: review?(hskupin) → review-
(Assignee)

Comment 8

9 months ago
Created attachment 8834550 [details] [diff] [review]
Path v_1.3, replacing in by "=="
Attachment #8834351 - Attachment is obsolete: true
Attachment #8834550 - Flags: review?(hskupin)
Comment on attachment 8834550 [details] [diff] [review]
Path v_1.3, replacing in by "=="

Review of attachment 8834550 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me now. Thanks!
Attachment #8834550 - Flags: review?(hskupin) → review+
Keywords: checkin-needed
(Assignee)

Comment 10

9 months ago
Thanks for all the help Henrik. Can you suggest me some other similar bugs to work upon?
Flags: needinfo?(hskupin)
Assignee: nobody → anjul.ten
Anjul, I was notified that your patch doesn't include any commit message. This is important to set before we can land your patch. I totally missed that part when doing the review due to the old review ui. Can you please come up with a good one? If you have problems to describe it please let me know. Otherwise we use the style "Bug %id% - %description%. r=whimboo".

Thanks.

We can check for further bugs once the above is done. Best would be if you can join us in the #automation channel. It works better to discuss there.
Flags: needinfo?(hskupin) → needinfo?(anjul.ten)
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 13

9 months ago
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Anjul, I was notified that your patch doesn't include any commit message.
> This is important to set before we can land your patch. I totally missed
> that part when doing the review due to the old review ui. Can you please
> come up with a good one? If you have problems to describe it please let me
> know. Otherwise we use the style "Bug %id% - %description%. r=whimboo".
> 

Oops, sorry. But this time, I've submitted the patch via mozreview with the commit message. Please check and approve.
(Assignee)

Updated

9 months ago
Status: NEW → ASSIGNED
Flags: needinfo?(anjul.ten)

Comment 14

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review112796

One minor thing to fix, but which would cause a backout. Then we can get it landed.

Btw great to see that you were able to setup Mozreview! It's way better now to review the patch! Thanks a lot.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:36
(Diff revision 1)
>          self.assertEqual(self.marionette.current_window_handle, self.start_tab)
>  
>          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, timeout=self.marionette.timeout.page_load).until(
> +                lambda mn: mn.get_url() == self.empty_page)

You indented this line by 8 chars now. This might give a linting error. Please fix it by using the default 4 chars.
Attachment #8834987 - Flags: review?(hskupin) → review+

Comment 15

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review112798

I would also suggest a better commit message like: "Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab".
Comment hidden (mozreview-request)

Comment 17

9 months ago
5 failures in 836 pushes (0.006 failures/push) were associated with this bug in the last 7 days.  
Repository breakdown:
* mozilla-inbound: 4
* mozilla-central: 1

Platform breakdown:
* linux64: 3
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1336022&startday=2017-02-06&endday=2017-02-12&tree=all
Attachment #8834987 - Attachment is obsolete: true
Attachment #8834550 - Attachment is obsolete: true

Comment 18

9 months ago
mozreview-review
Comment on attachment 8835961 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/111506/#review113232

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:36
(Diff revision 1)
>          self.assertEqual(self.marionette.current_window_handle, self.start_tab)
>  
>          self.marionette.switch_to_window(new_tab)
>          self.assertEqual(self.marionette.current_window_handle, new_tab)
>          Wait(self.marionette, timeout=self.marionette.timeout.page_load).until(
> -                lambda mn: mn.get_url() == self.empty_page)
> +            lambda mn: mn.get_url() == self.empty_page)

You added another commit to this patch. What you actually want is to amend the changes to your former commit. Please have a look at `hg histedit` to combine both commits.
Attachment #8835961 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8835961 - Attachment is obsolete: true
(Assignee)

Comment 20

9 months ago
Is there anything pending with this issue? I submitted an amendment to the original commit which resolves the issue. 

Thanks @whimboo for the help throughout.
Flags: needinfo?(hskupin)

Comment 21

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review114142

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:36
(Diff revision 2)
>          self.assertEqual(self.marionette.current_window_handle, self.start_tab)
>  
>          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, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: mn.get_url() == self.empty_page)

Interesting that I missed to mention that earlier in the review, but beside the callback parameter you also want to add a message parameter which explains why the test is failing. It should look like:

lambda: ...,
message="{} did not load after opening a new tab"

If we don't do this the timeout exception would contain no details which makes it hard to investigate the issue.

Thanks and sorry for it.
Attachment #8834987 - Flags: review+
(In reply to Anjul Tyagi from comment #20)
> Is there anything pending with this issue? I submitted an amendment to the
> original commit which resolves the issue. 

No, looks like I simply missed it.
Flags: needinfo?(hskupin)
(Assignee)

Comment 23

9 months ago
Hey Henrik, can you let me know how to unfold a previous commit. Because I was working on another bug, I dropped the commit for this bug.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

9 months ago
I pushed the change for review.
(Assignee)

Updated

9 months ago
Flags: needinfo?(hskupin)

Comment 26

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review114824

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_content.py:36
(Diff revision 3)
>          self.assertEqual(self.marionette.current_window_handle, self.start_tab)
>  
>          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, timeout=self.marionette.timeout.page_load).until(
> +            lambda mn: mn.get_url() == self.empty_page, message="{} did not load after opening a new tab")

This exceed the maximum line length and will give a linter failure. Please move the message param to the next line. Also it misses the variable for the formatting placeholder.
Attachment #8834987 - Flags: review?(hskupin) → review-
(Assignee)

Comment 27

9 months ago
I didn't understand what you meant by "variable for formatting placeholder". Can you please help me with this?
Flags: needinfo?(hskupin)
The message we want to display in case of an exception has to contain the expected target page. As such you have the '{}' characters in the string. This is used for formatting like "{}".format(variable) - which puts the variable's value into the string. I hope that helps you to clear up the question.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request)

Comment 30

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review114896

Code-wise it looks fine now. But your commit message is garbled with an additional Mozreview commit id before the real commit message. You most likely want to replace the line D2Cx1ntdC35 with ExmUG2vApbl, and remove the first to lines.
Attachment #8834987 - Flags: review?(hskupin) → review+
Comment hidden (mozreview-request)

Comment 32

9 months ago
mozreview-review
Comment on attachment 8834987 [details]
Bug 1336022 - Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab.

https://reviewboard.mozilla.org/r/110716/#review114960

Thanks that looks fine now.

Comment 33

9 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf3bd5215ecf
Fix test_window_handles_after_opening_new_tab by waiting for the page loaded in the new tab. r=whimboo

Comment 34

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf3bd5215ecf
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
Thank you Anjul for getting this test failure fixed! If you are interested in working on something else and to learn more, please let me know. Thanks.
See Also: → bug 1343039
Depends on: 1344647
(Assignee)

Comment 36

9 months ago
(In reply to Henrik Skupin (:whimboo) from comment #35)
> Thank you Anjul for getting this test failure fixed! If you are interested
> in working on something else and to learn more, please let me know. Thanks.

Thanks Henrik for helping me learn the whole process. It was a great experience of fixing my first bug.
However, presently I'm working on Balrog but I would love to fix similar bugs like 134467 which can help me with my practice.
This test-only patch blocks another patch from bug 1344647 being landed. So if possible please uplift it to beta and esr52. Thanks.
Whiteboard: [lang=py] → [lang=py][checkin-needed-beta][checkin-needed-esr52]
status-firefox53: unaffected → affected
status-firefox-esr52: --- → affected

Comment 38

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2a28f378103f
status-firefox53: affected → fixed
Whiteboard: [lang=py][checkin-needed-beta][checkin-needed-esr52] → [lang=py][checkin-needed-esr52]

Comment 39

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/6f7195344f2a
status-firefox-esr52: affected → fixed
Whiteboard: [lang=py][checkin-needed-esr52] → [lang=py]
You need to log in before you can comment on or make changes to this bug.