Closed
Bug 1336022
Opened 7 years ago
Closed 7 years ago
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 '
Categories
(Testing :: Marionette Client and Harness, defect)
Tracking
(firefox52 unaffected, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox-esr52 | --- | fixed |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: anjul.ten, Mentored)
References
Details
(Keywords: intermittent-failure, Whiteboard: [lang=py])
Attachments
(1 file, 4 obsolete files)
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
Comment 1•7 years ago
|
||
[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•7 years ago
|
Mentor: hskupin
Whiteboard: [lang=py]
Assignee | ||
Comment 2•7 years ago
|
||
As per your suggestion for the bug #1334996, Can I work on this one ?
Flags: needinfo?(hskupin)
Comment 3•7 years ago
|
||
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•7 years ago
|
||
Attachment #8833982 -
Flags: review?(hskupin)
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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 7•7 years ago
|
||
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•7 years ago
|
||
Attachment #8834351 -
Attachment is obsolete: true
Attachment #8834550 -
Flags: review?(hskupin)
Comment 9•7 years ago
|
||
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+
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•7 years ago
|
||
Thanks for all the help Henrik. Can you suggest me some other similar bugs to work upon?
Flags: needinfo?(hskupin)
Updated•7 years ago
|
Assignee: nobody → anjul.ten
Comment 11•7 years ago
|
||
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•7 years 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•7 years ago
|
Status: NEW → ASSIGNED
Flags: needinfo?(anjul.ten)
Comment 14•7 years 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•7 years 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 hidden (Intermittent Failures Robot) |
Updated•7 years ago
|
Attachment #8834987 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8834550 -
Attachment is obsolete: true
Comment 18•7 years 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•7 years ago
|
Attachment #8835961 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 years 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•7 years 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+
Comment 22•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
I pushed the change for review.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(hskupin)
Comment 26•7 years 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•7 years ago
|
||
I didn't understand what you meant by "variable for formatting placeholder". Can you please help me with this?
Flags: needinfo?(hskupin)
Comment 28•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf3bd5215ecf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
Comment 35•7 years ago
|
||
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.
Assignee | ||
Comment 36•7 years 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.
Comment 37•7 years ago
|
||
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]
Updated•7 years ago
|
Comment 38•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2a28f378103f
Whiteboard: [lang=py][checkin-needed-beta][checkin-needed-esr52] → [lang=py][checkin-needed-esr52]
Comment 39•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/6f7195344f2a
Whiteboard: [lang=py][checkin-needed-esr52] → [lang=py]
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 40•1 year ago
|
||
Moving bug to Testing::Marionette Client and Harness component per bug 1815831.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•