Closed Bug 1259055 Opened 8 years ago Closed 8 years ago

Lots of Marionette unit tests do not clean-up left open windows and tabs

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1259055 - Update Marionette unit tests to use correct handles for chrome windows.
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1259055 - Use window management class for handling of new tabs.
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1259055 - Use window management class for handling of new windows.
58 bytes, text/x-review-board-request
impossibus
: review+
Details
testcase for not finding new tab
1.83 KB, text/plain
Details 
Any of the window handling unit tests in Marionette do not clean-up the state of the browser after they are run. This can cause trouble in case of assertions during tests. So tabs and windows can still be open and will block following tests.

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit/

The unit tests can be run via: "mach marionette-test"

Maja, I wonder if that would be a good bug for a contributor.
Flags: needinfo?(mjzffr)
My impression is that current window management tests do bleed state, but that they are “saved” somewhat by the imposed running order of the files.  Shortly after the window management tests are run, the browser is restarted into a fresh state.

This is obviously all accidental and we should avoid non-deterministic tests.  Restarting the browser after each test is an obvious solution but increases the end-to-end time to an unacceptably high value.
Each test can safe the original chrome window and/or window handle and close every other window and tab.
(In reply to Henrik Skupin (:whimboo) from comment #2)
> Each test can safe the original chrome window and/or window handle and close
> every other window and tab.

Yes.  And I’d rather we be specific about this in each test than make this a default in the MarionetteTestCase superclass.
Right. For firefox-ui-tests we have at least a helper in the general testcase class which checks for leaks and report that accurately. It's a good indicator. Not sure what you think about something like that.

http://mxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py#20
I've tentatively added this to my private tag of outreachy bugs.
Flags: needinfo?(mjzffr)
Mentor: hskupin
Summary: Marionette test_window_* unit tests do not clean-up left open windows and tabs → Lots of Marionette unit tests do not clean-up left open windows and tabs
Whiteboard: [lang=py]
Blocks: 1289669
I think that this issue is causing a couple of issues for our Marionette tests those days. I feel it is kinda important right now to get it fixed. So I will do so that it hopefully helps us to cut down the list of Mn(-e10s) failures per push.
Assignee: nobody → hskupin
Mentor: hskupin
Status: NEW → ASSIGNED
I'm currently blocked with any kind of refactoring due to the hangs I see and as reported as bug 1294456.
Depends on: 1294456
I just saw an example of this problem in the wild: TestSwitchFrameChrome opens a modal window [1] which doesn't close right after the test is done, and that affects other window-related tests if they run too soon after TestSwitchFrameChrome. For example, tests in test_window_switching.py, test_screenshot.py fail if run immediately after test_switch_frame_chrome.py with errors like: "NoSuchWindowException: No such content frame; perhaps the listener was not registered?"

[1] https://dxr.mozilla.org/mozilla-central/rev/bd7645928990649c84609d3f531e803c2d41f269/testing/marionette/harness/marionette/tests/unit/test_switch_frame_chrome.py#14
Attached file testcase (changing outer window id) (obsolete) — 
While I continued working on this bug locally for some minutes I ran into the next problem with window handles. As it looks like at least about:preferences changes the outer window id of the current tab and it stays that way until we load another web page:

> * after navigate local page: [u'2147483649']
> * after navigate preferences: [u'10']
> * after navigate blank: [u'10']
> * after navigate local page: [u'4294967297']

I assume this is due to some remoteness changes? Mike, do you have an idea? Is that only happening for about:preferences or could it happen for any page or chrome in content page?
Flags: needinfo?(mconley)
David, the attached mozreview patch is heavily WIP. It would just be good to know for now if the mixin solution is fine with you. Thanks.
Flags: needinfo?(dburns)
Comment on attachment 8788153 [details]
Bug 1259055 - Allow MRO for Marionette testcase classes.

https://reviewboard.mozilla.org/r/76744/#review74846

::: testing/marionette/harness/marionette/tests/unit/test_about_pages.py:23
(Diff revision 1)
> +class TestAboutPages(MarionetteTestCase, WindowManagementMixin):
> +
>      def setUp(self):
> -        MarionetteTestCase.setUp(self)
> +        super(TestAboutPages, self).setUp()
> +
> +        self.assertEqual(len(self.start_tabs), 1)

we shouldn't be doing asserts in setups and teardowns

::: testing/marionette/harness/marionette/tests/unit/test_about_pages.py:86
(Diff revision 1)
>              urlbar.send_keys(self.mod_key + 'x')
>              urlbar.send_keys(self.remote_uri + Keys.ENTER)
>  
>          self.wait_for_condition(lambda mn: mn.get_url() == self.remote_uri)
>  
> -    def test_hang(self):
> +    def tst_hang(self):

dont forget to put this back to a test
Flags: needinfo?(dburns)
Comment on attachment 8788153 [details]
Bug 1259055 - Allow MRO for Marionette testcase classes.

https://reviewboard.mozilla.org/r/76744/#review74846

> we shouldn't be doing asserts in setups and teardowns

Is that something special we setup or why is that the case? I haven't seen that this is forbidden:
https://docs.python.org/2/library/unittest.html#unittest.TestCase.setUp
Comment on attachment 8788153 [details]
Bug 1259055 - Allow MRO for Marionette testcase classes.

https://reviewboard.mozilla.org/r/76744/#review74846

> Is that something special we setup or why is that the case? I haven't seen that this is forbidden:
> https://docs.python.org/2/library/unittest.html#unittest.TestCase.setUp

It's just a poor practise. Set Up methods are there to make sure that the test methods that follow are in the correct state. Asserting on something and instead of making sure the test is correct to start feels wrong.
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Created attachment 8788148 [details]
> testcase (changing outer window id)
> 
> While I continued working on this bug locally for some minutes I ran into
> the next problem with window handles. As it looks like at least
> about:preferences changes the outer window id of the current tab and it
> stays that way until we load another web page:
> 
> > * after navigate local page: [u'2147483649']
> > * after navigate preferences: [u'10']
> > * after navigate blank: [u'10']
> > * after navigate local page: [u'4294967297']
> 
> I assume this is due to some remoteness changes? Mike, do you have an idea?
> Is that only happening for about:preferences or could it happen for any page
> or chrome in content page?

Yes, what you're seeing is the result of remoteness changes. There are several types of URLs that our remoteness switching machinery considers:

1) URLs that will definitely be loaded in a remote browser
2) URLs that will definitely be loaded in a non-remote browser
3) URLs that can load in either, and will not cause a remoteness switch.

See https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Which_URIs_load_where for more details about what goes where.

Looking at your log, this is what's happening:

1) Navigate to a locally hosted webpage, which we can load in the content process, so the browser is running remotely
2) Navigate to about:preferences, which MUST run in the parent, so we do a remoteness switch
3) Navigate to about:blank from about:preferences. about:blank is in group (3), so we don't switch remoteness, but stay in the parent process.
4) Navigate to a locally hosted webpage, which MUST run in the child, so we do a remoteness switch.

I hope that clears up what's happening for you.
Flags: needinfo?(mconley)
Blocks: 1075383
Blocks: 1283920
Blocks: 1293073
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #15)
> I hope that clears up what's happening for you.

Yes, thanks a lot for this detailed information Mike! So my thinking now is to first check if the original tab is still around. If yes, lets close all others. But if it is not, take the current one instead and close all others.
Blocks: 1293074
Blocks: 1231446
So I played around with that a bit more today. And my current thinking is that using the outer window id for identifying tabs might not be a good idea anymore. With e10s enabled the id can always change simply by navigating through URLs, and tests will fail once they try to re-enable a formerly saved-off tab handle.

Mike, do you know of a property which is more reliable and persistent? If there is none we may have to set our own ID instead?
Flags: needinfo?(mconley)
I would also like to get feedback from Andreas in regards of a permanent ID for tabs.
Flags: needinfo?(ato)
One other thing is that most likely only our about: pages are causing a remoteness change given that they run as non-remote. We could special case the unit tests and ensure to always return to remoteness at the end of the test. This at least works fine for me, but we should keep that in mind when testing about: pages.
Depends on: 1305659
(In reply to Henrik Skupin (:whimboo) from comment #17)
> With e10s enabled the id can always change simply by navigating
> through URLs, and tests will fail once they try to re-enable a formerly
> saved-off tab handle.

Why does the outerWindowID change on navigation?

> Mike, do you know of a property which is more reliable and persistent? If
> there is none we may have to set our own ID instead?

I don’t know of anything more persistent.  We can probably generate our own UUID window handle, like you propose, and I don’t see any problems with that.  However, if Gecko has something we can re-use I would prefer that rather than having to roll our own lookup table.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #19)
> One other thing is that most likely only our about: pages are causing a
> remoteness change given that they run as non-remote. We could special case
> the unit tests and ensure to always return to remoteness at the end of the
> test. This at least works fine for me, but we should keep that in mind when
> testing about: pages.

We should also take into account that a consumer of WebDriver might navigate to about:-pages, and we need to ensure window handles stay consistent also for these.  So special casing only the tests without fixing the implementation is not a winning strategy here.
I think we should special case about: pages in our unit tests. Those are really the only ones which seem to not use remoteness, well except about:blank. Maybe Mike could still tell us, if there is already a more stable ID available.

Btw. while working on those unit tests I noticed that those are kinda doing wrong things, eg. when working with chrome windows we use methods operating on tabs. Not sure why those things have been landed earlier, but that should be all fixed. As of right now, I feel that lots of the tests are not doing what they are expected to do.
I'm not entirely sure what kind of ID you're looking for, to be honest. <xul:browser>'s aren't given an ID number, if that's what you're asking. They are, however, given a "permanentKey", which is an empty object that we sometimes use to differentiate <xul:browser>'s. These permanentKey's are, for example, transferred when a browser is torn out to a new window.

They can be accessed via browser.permanentKey. But they're just empty objects - best used in things like WeakMap's and WeakSet's.

I agree that the outerWindowID is not very reliable, since browser's can flip remoteness, which will change the outerWindowID. Remoteness flips do not affect permanentKey.

Does that help?
Flags: needinfo?(mconley)
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1311041 about not using outerWindowID based on mconley’s last comment.
Depends on: 1311350
As noticed while working on updating the tests, both methods close() and closeChromeWindow() in Marionette server do not wait until the underlying window has really been closed. I will refrain from working around that in the upcoming patch for this bug. Instead I filed bug 1311350 to get this correctly fixed on the server side.
The patch is still in a WIP state. What's missing is the correct handling of new chrome windows. I think that I should be able to finish this today.
Depends on: 1311676
Interesting that my latest try build failed when comparing the window title:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6276b3a4da&filter-tier=1&filter-tier=2&filter-tier=3

> AssertionError: u'We Arrive Here - Nightly' != 'We Arrive Here'

Why is Nightly part of the title with a try build?
(In reply to Henrik Skupin (:whimboo) from comment #34)
> > AssertionError: u'We Arrive Here - Nightly' != 'We Arrive Here'
> 
> Why is Nightly part of the title with a try build?

Problem solved. Before my changes self.marionette.title was running in content scope, now I had it in chrome scope. And as such the window title includes the "- Nightly" suffix. Making use of using_context('content') works just fine.
Last try build shows a race when opening a new window only on Linux:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b4863123907

With the latest commit I added a `loaded()` method to ensure we wait until the new chrome window has been fully loaded. I picked this from Puppeteer.
The last failure which seems to remain here and which is a race is:

>  TEST-UNEXPECTED-ERROR | test_window_handles.py TestWindowHandles.test_chrome_windows | NoSuchElementException: Unable to locate element: new-tab 

Once this has been fixed the patch should be ready for review.
Attachment #8788153 - Attachment is obsolete: true
(In reply to Henrik Skupin (:whimboo) from comment #40)
> The last failure which seems to remain here and which is a race is:
> 
> >  TEST-UNEXPECTED-ERROR | test_window_handles.py TestWindowHandles.test_chrome_windows | NoSuchElementException: Unable to locate element: new-tab 

With a fix for this failure in place the try builds showed another failure for Linux debug only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3444ffaba8bf

The problem with that is that we do not correctly load expected test pages. A fix with a try push is working:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3444ffaba8bf

I will finish up this patch and request another full try build just to be sure it works.
Adding some debug code for the above mentioned patch seems to have been the reason why the tests were passing! Removing it caused the failure to re-appear. :( Looks like I have to do some more testing on Linux debug builds on Buildbot where it is failing.
Actually the debug builds are showing what's going wrong here:

1477322162221	Marionette	TRACE	conn898 -> [0,11,"findElement",{"using":"id","value":"new-tab"}]
1477322162252	Marionette	TRACE	conn898 <- [1,11,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"064adf67-9368-4398-be83-4b8c83b3644c","ELEMENT":"064adf67-9368-4398-be83-4b8c83b3644c"}}]
1477322162290	Marionette	TRACE	conn898 -> [0,12,"clickElement",{"id":"064adf67-9368-4398-be83-4b8c83b3644c"}]
JavaScript error: chrome://browser/content/browser.js, line 180: TypeError: can't access dead object
JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 197: TypeError: can't access dead object

So we are working on a dead object here which is the new-tab link. I will check tomorrow what's wrong.
Depends on: 1312736
I can reproduce this test failure on the loaner now with only those two tests running:

TEST-START | test_window_management.py TestSwitchWindow.test_windows
TEST-PASS | test_window_management.py TestSwitchWindow.test_windows | took 7523ms
TEST-START | test_window_handles.py TestWindowHandles.test_tab_and_window_handles
loop: handles before: [u'18']
handles now: [u'18', u'33']
loop: handles before: [u'18', u'33']
handles now: [u'18', u'33']
handles now: [u'18', u'33']
handles now: [u'18', u'33']
handles now: [u'18', u'33']
[..]
TEST-UNEXPECTED-ERROR | test_window_handles.py TestWindowHandles.test_tab_and_window_handles | TimeoutException: Timed out after 5.0 seconds with message: No new tab has been opened

Interesting here is that `test_windows` is closing the original chrome window and keeps working with a formerly opened second one. When I skip this test all is working fine. Also I noticed that the expected window we are trying to open is not a browser window but a popup without any chrome elements. Maybe that is the reason why we fail to detect the content window handle?
The problem why we are failing here on the test slave is the following line:

> execute_script(" window.open('chrome://browser/content/browser.xul'); ")

When I remove the chrome URL and simply call `window.open()` all is working fine and the tab of the newly opened window later in this test will be found.

Andreas, as we can see this is a Linux only issue which seems to only happen on test slaves. May question is if I can simply change the call to the above convention for this test. I'm also not sure if it would make sense to continue investigating this problem. I could at least file a new bug with this testcase. What do you think?
Attachment #8788148 - Attachment is obsolete: true
Flags: needinfo?(ato)
I don’t know why this happens, especially not only on one configuration, but it might be worth checking if this is a thing with officially branded builds or not.

I also don’t have any strong opinions about how to work around this.  I feel perhaps it is not a big priority to dive into further as it’s a fairly small edge case.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #54)
> I don’t know why this happens, especially not only on one configuration, but
> it might be worth checking if this is a thing with officially branded builds
> or not.

No, it works all fine down to release. So it might be specific to this test machine. Keep in mind that the linux testers still use Ubuntu 12.04 while linux 64 is using 16.04. This might be a reason.

> I also don’t have any strong opinions about how to work around this.  I feel
> perhaps it is not a big priority to dive into further as it’s a fairly small
> edge case.

What I have just seen is that using window.open() with browser.xul as URL we get a new browser window with the browser also opened inside the tab. Given that this is not what we need here - a new browser window is enough - I will remove it.
Blocks: 1313593
Blocks: 1311657
Comment on attachment 8802503 [details]
Bug 1259055 - Update Marionette unit tests to use correct handles for chrome windows.

https://reviewboard.mozilla.org/r/86888/#review88626
Attachment #8802503 - Flags: review?(mjzffr) → review+
Comment on attachment 8802504 [details]
Bug 1259055 - Use window management class for handling of new tabs.

https://reviewboard.mozilla.org/r/86890/#review88628

::: testing/marionette/harness/marionette/tests/unit/mixin_utils.py:11
(Diff revision 6)
> +import unittest
> +
> +from marionette_driver import By, Wait
> +
> +
> +class WindowManagementMixin(unittest.TestCase):

This should go in either testing/marionette/harness/marionette/marionette_test/testcases.py or maybe testing/marionette/harness/marionette/runner/mixins  so that it's more discoverable and useful to others.

It's not really a mixin since it relies on self.marionette from MarionetteTestCase: it should just inherit from MarionetteTestCase.
Attachment #8802504 - Flags: review?(mjzffr) → review-
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review88630
Attachment #8802881 - Flags: review?(mjzffr)
(In reply to Maja Frydrychowicz (:maja_zf) from comment #61)
> > +class WindowManagementMixin(unittest.TestCase):
> 
> This should go in either
> testing/marionette/harness/marionette/marionette_test/testcases.py or maybe
> testing/marionette/harness/marionette/runner/mixins  so that it's more
> discoverable and useful to others.

I thought that it was something David didn't wanted to have in base Marionette. Maybe I was wrong or he might change his mind now? David, what do you think about it?

> It's not really a mixin since it relies on self.marionette from
> MarionetteTestCase: it should just inherit from MarionetteTestCase.

I cannot do this because afair it would be in conflict with the CommonTestCase class. But I might be wrong given that by that time MRO wasn't fully implemented. I will have a look again tomorrow.
Flags: needinfo?(dburns)
(In reply to Henrik Skupin (:whimboo) from comment #63)
> (In reply to Maja Frydrychowicz (:maja_zf) from comment #61)
> > > +class WindowManagementMixin(unittest.TestCase):
> > 
> > This should go in either
> > testing/marionette/harness/marionette/marionette_test/testcases.py or maybe
> > testing/marionette/harness/marionette/runner/mixins  so that it's more
> > discoverable and useful to others.
> 
> I thought that it was something David didn't wanted to have in base
> Marionette. Maybe I was wrong or he might change his mind now? David, what
> do you think about it?

I think putting it in /mixins would be better but I agree with the 2nd comment from Maja that it doesnt feel like a mixin but if you cant do it straight away, putting it in /mixin is a good place for it.

> 
> > It's not really a mixin since it relies on self.marionette from
> > MarionetteTestCase: it should just inherit from MarionetteTestCase.
> 
> I cannot do this because afair it would be in conflict with the
> CommonTestCase class. But I might be wrong given that by that time MRO
> wasn't fully implemented. I will have a look again tomorrow.
Flags: needinfo?(dburns)
(In reply to Maja Frydrychowicz (:maja_zf) from comment #61)
> ::: testing/marionette/harness/marionette/tests/unit/mixin_utils.py:11
> (Diff revision 6)
> > +import unittest
> > +
> > +from marionette_driver import By, Wait
> > +
> > +
> > +class WindowManagementMixin(unittest.TestCase):
> 
> This should go in either
> testing/marionette/harness/marionette/marionette_test/testcases.py or maybe
> testing/marionette/harness/marionette/runner/mixins  so that it's more
> discoverable and useful to others.
>
> It's not really a mixin since it relies on self.marionette from
> MarionetteTestCase: it should just inherit from MarionetteTestCase.

Why should it not be a real mixin class? Mixins are used to add optional features which then can be used by different classes. A mixin class is not required to be fully implemented but can rely on properties and methods as available by the class they are mixed in. If you read something different about it please let me know. But the above is what I understand from mixins and how those have been used in other projects I participated in. Btw. also see the mixins in runner/mixins for examples. Those also use self.marionette but even have object as base class.

Using MarionetteTestCase as base class will not work, because you won't be able to mixin the WindowManagement class due to MRO conflicts (you actually would try to mixin a subclass):

> TypeError: Error when calling the metaclass bases
>      Cannot create a consistent method resolution
>  order (MRO) for bases WindowManagementMixin, MarionetteTestCase

As such I'm also not that happy to make it a subclass of marionette.CommonTestCase. The real underlying base class should be unittest.TestCase. I see that other mixin classes (as pointed out above) use the object class, but this doesn't work here because TestCase.setUp() doesn't support a call to super():

>    def setUp(self):
>        "Hook method for setting up the test fixture before exercising it."
>        pass

I will wait with an update of this patch until we reached agreement on the above. But good to see that we can consider to move it to a general place in Marionette now.
Flags: needinfo?(mjzffr)
(In reply to Henrik Skupin (:whimboo) from comment #65)
> (In reply to Maja Frydrychowicz (:maja_zf) from comment #61)
> > ::: testing/marionette/harness/marionette/tests/unit/mixin_utils.py:11
> > (Diff revision 6)
> > > +import unittest
> > > +
> > > +from marionette_driver import By, Wait
> > > +
> > > +
> > > +class WindowManagementMixin(unittest.TestCase):
> > 
> > This should go in either
> > testing/marionette/harness/marionette/marionette_test/testcases.py or maybe
> > testing/marionette/harness/marionette/runner/mixins  so that it's more
> > discoverable and useful to others.
> >
> > It's not really a mixin since it relies on self.marionette from
> > MarionetteTestCase: it should just inherit from MarionetteTestCase.
> 
> Why should it not be a real mixin class? Mixins are used to add optional
> features which then can be used by different classes. A mixin class is not
> required to be fully implemented but can rely on properties and methods as
> available by the class they are mixed in. If you read something different
> about it please let me know. But the above is what I understand from mixins
> and how those have been used in other projects I participated in. Btw. also
> see the mixins in runner/mixins for examples. Those also use self.marionette
> but even have object as base class.

Yeah, there are different opinions out there about mixins (e.g. mix-in classes "don't define their own instance attributes nor require their __init__ to be called"), but that's getting into a discussion about semantics, which isn't necessarily productive. 

My point is that I think you can achieve what you want more cleanly by defining a custom test case class, `class WindowManagementTestCase(MarionetteTestCase)`, and then use it as `TestWhatever(WindowManagementTestCase)`.

Or if you really want to use multiple inheritance in the test modules, try:
`class WindowManagementMixin(unittest.TestCase)` or `class WindowManagementMixin(MarionetteTestCase)`

with the following in test_whatever.py:

`class TestSomething(WindowManagementMixin, MarionetteTestCase):`

> 
> Using MarionetteTestCase as base class will not work, because you won't be
> able to mixin the WindowManagement class due to MRO conflicts (you actually
> would try to mixin a subclass):
> 
> > TypeError: Error when calling the metaclass bases
> >      Cannot create a consistent method resolution
> >  order (MRO) for bases WindowManagementMixin, MarionetteTestCase
> 


As above, switching the class order avoids the error you describe, regardless of whether WindowManagementMixin is based on MarionetteTestCase or unittest.TestCase:

`class MyTestCase(WindowManagementMixin, MarionetteTestCase):`
Flags: needinfo?(mjzffr)
Comment on attachment 8802504 [details]
Bug 1259055 - Use window management class for handling of new tabs.

https://reviewboard.mozilla.org/r/86890/#review89854
Attachment #8802504 - Flags: review?(mjzffr) → review+
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review89856

When I look at the squashed diff, it seems that this is still left over: testing/marionette/harness/marionette/tests/unit/mixin_utils.py
Attachment #8802881 - Flags: review?(mjzffr) → review-
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review89998

Strange. Somehow this file really made it into the final commit. I have removed it now. Given that you haven't had any other complains I assume you are fine with the rest of the patch?
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review90108

r+wc

::: testing/marionette/harness/marionette/tests/unit/test_chrome.py:43
(Diff revision 9)
> -            self.marionette.switch_to_window(handles[0])
> -            self.assertRaises(NoSuchElementException, self.marionette.find_element, By.ID, 'dek')
> -
> -            # Clean up the window
> -            self.marionette.close()
> -            self.marionette.switch_to_window(start_handle)
> +        self.marionette.switch_to_window(new_window)
> +
> +        try:
> +            # Force an exception in the non-browser window, which should
> +            # not cause a hang when closing it.
> +            self.marionette.find_element(By.ID, 'dck')

This change removes assertRaises, so if `find_element` doesn't raise any exception at all, the test will pass. Is that what we want?

::: testing/marionette/harness/marionette/tests/unit/test_pagesource_chrome.py:14
(Diff revision 9)
> -        self.win = self.marionette.current_chrome_window_handle
> -        self.marionette.execute_script("window.open('chrome://marionette/content/test.xul', 'foo', 'chrome,centerscreen');")
> -        self.marionette.switch_to_window('foo')
> -        self.assertNotEqual(self.win, self.marionette.current_chrome_window_handle)
>  
> +        def open_with_js():

These functions are repeated in many places. It might be a good idea to move them all to one place for reuse, but it could be done in another bug.
Attachment #8802881 - Flags: review?(mjzffr) → review+
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review90108

> This change removes assertRaises, so if `find_element` doesn't raise any exception at all, the test will pass. Is that what we want?

The underlying problem here is bug 1141519 which I filed a while ago. The test as currently in the tree is actually not testing what it should do. The exception as raised is irrelevant for itself. What should be tested is that the other chrome window can still be closed and doesn't cause a hang.

But I see that it might still not be perfect. I will check it again.

> These functions are repeated in many places. It might be a good idea to move them all to one place for reuse, but it could be done in another bug.

The tests for handling chrome windows will need a refactoring, so we can indeed do this when it gets done.
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review90108

> The underlying problem here is bug 1141519 which I filed a while ago. The test as currently in the tree is actually not testing what it should do. The exception as raised is irrelevant for itself. What should be tested is that the other chrome window can still be closed and doesn't cause a hang.
> 
> But I see that it might still not be perfect. I will check it again.

I discussed this with Maja via IRC and the updated version is fine now.
Comment on attachment 8802881 [details]
Bug 1259055 - Use window management class for handling of new windows.

https://reviewboard.mozilla.org/r/87158/#review90154

With the last update I changed the base class of the WindowManager mixin from `unittest.TestCase` to just `object`. That was missing before.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/591875044a6b
Update Marionette unit tests to use correct handles for chrome windows. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/e9b68bce5475
Use window management class for handling of new tabs. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/9c62a8b447f7
Use window management class for handling of new windows. r=maja_zf
Blocks: 1292797
Blocks: 1293071
Blocks: 1303640
Blocks: 1231473
In the case of this patch applies cleanly on aurora we should get it uplifted to reduce the amount of intermittent test failures for Marionette unit tests in handling chrome windows and tabs.
status-firefox51: --- → fix-optional
Whiteboard: [lang=py] → [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: