1146990 - Add more details about which window handles are leaked in tearDown of a FirefoxTestCase

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Log of test run that shows the AssertionError

This was encountered once so far during a test that is run about once per day.
 
The log reports "AssertionError: A test must not leak window handles. This test started the browser with 1 open top level browsing contexts, but ended with 2.", but the test itself does not open or close any windows or tabs. (See URL field of this bug for test source code.)

It would be helpful if the error message described the open windows.

Comment 1

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

The underlying problem here is that updates are not disabled! That's why Firefox comes up with an update prompt after a while. Given that your tests run about 1h, this is very likely. So this specific case will be fixed via my patch on bug 1129843.

But what we need here, is to add more details about which handles leaked. The information we currently have is not sufficient.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

I have a commit on a branch here that prints out the window type and url of potentially leaked tabs/windows opened by a test: https://github.com/chmanchester/firefox-ui-tests/commit/f9701aa18a1d82449af407dafd4ca9307998e670

Maybe you can apply this to your harness to help track this down if you'd rather not update the submodule right now.

Comment 3

[Alex Kosiacka]

Hi!
I would love to do this bug, however I'm not quite sure that I know what you want me to do as it seems to me that ChrisManchester (comment 2) already submitted a patch that seems to resolve the issue. Is there an issue with his solution that you'd like me to fix, or does it just need to be formally submitted as an actual patch, and not a link to a commit?

Thanks for your help :-)

Comment 4

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

Hi Alex, you are welcome to pick up this bug. I would be happy to mentor you through the whole process.

So the patch from Chris only added the basic feature which checks for left-open window handles. It's not clear by checking the output which windows actually were left open. So one idea could be to print the window handles which haven't been closed. But given that those will vary during the lifetime of a full test run it might be better to actually print the corresponding window type or the window class name which is in use in our tests:

https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_puppeteer/ui/windows.py#L117

Please let me know how much guidance you will need related to your current level of expertise.

Comment 5

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

Alex, I wonder who you come along with this bug. Do you have any questions in case you are stuck?

Comment 6

[Alex Kosiacka]

Hi Henrik, so sorry I've been really busy and didn't get a chance to look again until this evening. 
I have a question about the patch submitted by Chris again, it appears to already print the window types and URLS of the windows that are left open at the end of the test. I'm not quite sure how it's behaviour is different to what you're requesting, could you please clarify?

Comment 7

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

(In reply to Alex Kosiacka from comment #6)
> I have a question about the patch submitted by Chris again, it appears to
> already print the window types and URLS of the windows that are left open at
> the end of the test. I'm not quite sure how it's behaviour is different to
> what you're requesting, could you please clarify?

I'm not sure where you are looking at. The latest code we have only asserts for the correct number of open windows/tabs. If that fails we only get that window count but no window types or whatever else.

https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_ui_harness/testcases/base.py#L24

Comment 8

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

Alex, was my last comment helpful for you? If not maybe you can point out at which source you are looking at. That way we might be able to clarify the confusion. Thanks.

Comment 9

[Alex Kosiacka]

Hello,

Yes it was, sorry I was so unresponsive I just have a lot of deadlines at the moment. I'm so sorry, I will try and get working on this bug properly in the next few days. 

I was looking at the possible patch that was already submitted. But your comment made sense yes. I think I have a pretty good idea of what I need to do. 

Please accept my apologies for being so unprofessional. I will try to make a patch by next Monday if that's okay?

Thank you so much for your help, I really appreciate it.

Comment 10

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

Hi Alex. There is absolutely no need to excuse. I think that this is normal when you step into a new project. So don't worry. :) 

If you need any other help please let me know. Otherwise I'm looking forward to your patch or if it is only a proposal. Thanks.

Comment 11

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

Hi Alex, are you still working on this bug? If not please let us know. In case it's too hard for you we can certainly find another bug for you.

Comment 12

[Alex Kosiacka]

Hi Henrik,

I have been working on a patch, this is what I have so far. I have a couple of questions as it doesn't work quite right. At the moment if I open a couple of new tabs whilst the tests are running, the test harness correctly ids that there are additional windows open however it does not identify what the windows are correctly and instead of listing the tabs I've opened it just gives the youtube URLs used for the test. I have looked over my code but I can't find a solution, and as I'm not familiar with the internals of the test harness I'm wondering if you know why it might be doing this? Thanks for your help.

https://github.com/alekskosiacka/firefox-ui-tests/commit/ef2606e25e3f89001806849cd15109e492cbb425

Comment 13

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

Hi Alex, good to see that you have had some time to dive into this bug. 

Well, `self.marionette.get_url()` retrieves the location of the current window. If the window is accessed via content scope you will see URLs of the web page loaded. In chrome scope you will get a plain browser.xul reference which is also not that helpful. Best here would be to take the content scope location (in your case the youtube URLs) because you can way better map those to left-over tabs when comparing the failure with the test steps.

Otherwise I don't think the patch from Chris really applies well anymore. As in our code we only take care of left-over tabs but not chrome window handles which are handled differently. So we can keep this bug to only cover tabs and filing a new one once the bug has been fixed to get the same working for chrome windows (browser, page info, etc.): 

http://marionette-client.readthedocs.org/en/latest/reference.html?highlight=chrome_window#marionette_driver.marionette.Marionette.chrome_window_handles

That means for each window we get reported in the current code we should be able to directly switch to it and print the url while staying in content scope.

If the above is still a bit unclear to you please let me know and maybe (if it works for you) we can have a chat on IRC in the #automation channel (https://kiwiirc.com/client/irc%3A%2F%2Fmozilla.org%2Fautomation)?

Comment 14

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

Alex, I assume you do not have time anymore to continue on this bug? Would be nice if you could let me know. Thanks.

Comment 15

[Alex Kosiacka]

Hi Henrik,
Yes, sorry. The module I was doing them for finished and others started, so unfortunately I have no time to continue on this bug.

Comment 16

[Carim Tho (:ctho)]

Hi! Right now I'm trying to understand this bug and I'd like to work on it.

Comment 17

[Carim Tho (:ctho)]

Attachment: MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

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

Comment 18

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

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

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review37811

Thanks, Carim. I think this is a good start. There are a few changes to make. When you push to review again, please change your commit message to the following: "Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf;whimboo

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:32
(Diff revision 2)
> +               for tab in range(len(tabbar.tabs)):
> +                        self.logger.error("Tab opened but not closed by this test:\n\t URL: %s" %(tabbar.tabs[tab].location))  

Please always use 4 spaces to indent. Here you have a 3-space indentation, followed by a 9-space indentation.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:33
(Diff revision 2)
> -                             (self._start_handle_count, handle_count))
> +                             (self._start_handle_count, handle_count)) 
> +        #Include more information in case the test fails. Added the URL from the tabs opened and not closed.
> +        except AssertionError:
> +            if self._start_handle_count < handle_count:
> +               for tab in range(len(tabbar.tabs)):
> +                        self.logger.error("Tab opened but not closed by this test:\n\t URL: %s" %(tabbar.tabs[tab].location))  

You can simplify this message to something like "Remaining tab URLS: [list of urls here]"

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:81
(Diff revision 2)
>          self._start_handle_count = len(self.marionette.window_handles)
>          self.marionette.set_context('chrome')
>  
>          self.browser = self.windows.current
>          self.browser.focus()
> +

There are some extra "empty" changes here and there, probably whitespace. Could you go through your submission and make sure your commit only contains relevant changes? Use hg histedit to modify your commit.

Comment 20

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

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

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review37949

Much better. :) Here is some more feedback.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:32
(Diff revision 3)
> +                self.logger.error("Remaining Tabs URLs:")
> +                for tab in range(len(tabbar.tabs)):
> +                    print "\t\t\t\t\t\t%s\n" %(tabbar.tabs[tab].location)

Rather than printing each line individually, you can build up a string in your loop and then log it.

A basic sketch of what I mean:

```
if stuff:
    message = ['Remaining tabs: ']
    for tab in tabs:
       message.append(tab.location)
    self.logger.error('\n'.join(message))
```

I like the formatting you added with '\t', but just each URL on a separate line without the '\t' is fine.

Comment 22

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

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

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review38025

Looks good to me.

Comment 24

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

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review38105

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:21
(Diff revision 4)
>      """
>      def __init__(self, *args, **kwargs):
>          MarionetteTestCase.__init__(self, *args, **kwargs)
>  
>      def _check_and_fix_leaked_handles(self):
>          handle_count = len(self.marionette.window_handles)

Just noticed that we only cover content windows here but not chrome windows. I will file a follow-up bug to get those added as well.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:25
(Diff revision 4)
>      def _check_and_fix_leaked_handles(self):
>          handle_count = len(self.marionette.window_handles)
> +        tabbar = self.browser.tabbar
>  
>          try:
>              self.assertEqual(handle_count, self._start_handle_count,

Why don't we have a simple `if` condition here which checks for the number of open handles? With that we would not need the `except` block.

Also shouldn't the message with the list of tabs be better part of the assertion? A separate logging error doesn't seem to be the best fit for me.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:35
(Diff revision 4)
> +        except AssertionError:
> +            if self._start_handle_count < handle_count:
> +                message = ['Remaining Tabs URLs:']
> +                for tab in range(len(tabbar.tabs)):
> +                    message.append(tabbar.tabs[tab].location)
> +                self.logger.error('\n'.join(message))

How would that look like in the log output? I wonder if we should better concanate tab URLs with comma.

Comment 25

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40875/diff/4-5/

Comment 26

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

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review39471

I would like to see how the output looks like in case of a test which does not correctly close all tabs. The already triggered try server job will not tell us about that.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:22
(Diff revision 5)
>      def __init__(self, *args, **kwargs):
>          MarionetteTestCase.__init__(self, *args, **kwargs)
>  
>      def _check_and_fix_leaked_handles(self):
>          handle_count = len(self.marionette.window_handles)
> +        tabbar = self.browser.tabbar

I don't really see a value for that extra variable. It's not adding that much. Also if we would use it, everything in this method should make use of it.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:27
(Diff revision 5)
> +        tabbar = self.browser.tabbar
>  
>          try:
> -            self.assertEqual(handle_count, self._start_handle_count,
> -                             'A test must not leak window handles. This test started with '
> -                             '%s open top level browsing contexts, but ended with %s.' %
> +            message = ('A test must not leak window handles. This test started with '
> +                       '%s open top level browsing contexts, but ended with %s.'
> +                       ' Remaining Tabs URLs:')%(self._start_handle_count, handle_count)

nit: Please separate `%` with spaces. The same for other changes in that file.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:30
(Diff revision 5)
> -            self.assertEqual(handle_count, self._start_handle_count,
> -                             'A test must not leak window handles. This test started with '
> -                             '%s open top level browsing contexts, but ended with %s.' %
> -                             (self._start_handle_count, handle_count))
> +            message = ('A test must not leak window handles. This test started with '
> +                       '%s open top level browsing contexts, but ended with %s.'
> +                       ' Remaining Tabs URLs:')%(self._start_handle_count, handle_count)
> +            if (tabbar.tabs):
> +                for tab in range(len(tabbar.tabs)):
> +                    message+= '\n%s' %tabbar.tabs[tab].location

When I check this closely I wonder if it is the correct approach. This code assumes that all handles are part of the first browser window, and then lists all of its tabs by ignoring the tab which was already open during `setUp()`. This will raise confusion.

We should better iterate over all window handles (which are tabs - content windows) and get the URL via `self.marionette.get_url()` for each of those remaining ones.

Therefore we should not store the original handle count but the list of handles. Then we can iterate through the current (remaining) window list in that method and retrieve the URL as described above.

Beside that I wonder if we can add the urls with comma as separator. Using new lines will not play that well with logging output.

Comment 27

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40875/diff/5-6/

Comment 28

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

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review39901

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:29
(Diff revision 6)
> -            self.assertEqual(handle_count, self._start_handle_count,
> -                             'A test must not leak window handles. This test started with '
> -                             '%s open top level browsing contexts, but ended with %s.' %
> -                             (self._start_handle_count, handle_count))
> +            message = ('A test must not leak window handles. This test started with '
> +                       '%s open top level browsing contexts, but ended with %s.'
> +                       ' Remaining Tabs URLs:') %(self._start_handle_count, handle_count)
> +            for tab in self.marionette.window_handles:
> +            	with self.marionette.using_context('content'):
> +                	message += ' %s,' %self.marionette.get_url()

You are actually not doing a comparison here. So it would add all existing tabs to the message, even those which were open when the test started.

As mentioned in my last review we should not safe off the `start_handle_count` but keep an array of `start_window_handles`, which we can use here for comparison.

Further please also check the indentation again. You seem to still use tabs and not 4 spaces for it.

Comment 29

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40875/diff/6-7/

Comment 30

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

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

https://reviewboard.mozilla.org/r/40875/#review40109

Thank you for the update! Functionality wise it looks fine. Please don't be afraid from the lot comments. Most of them are minor or suggest improvements. Please check them.

Given that I will be away from now on, I would suggest that you continue with Maja as reviewer here, so that you can finish this patch soon. Thanks for working on it!

::: testing/firefox-ui/tests/puppeteer/test_tabbar.py:15
(Diff revision 7)
>  class TestTabBar(FirefoxTestCase):
>  
>      def tearDown(self):
> -        try:
> -            self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
> -        finally:
> +        #try:
> +            #self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
> +        #finally:

The changes in that file are most likely left overs from testing? Please make sure to check the full diff before updating the mozreview. It would safe us an additional round of reviews in the future.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:24
(Diff revision 7)
>  
>      def _check_and_fix_leaked_handles(self):
>          handle_count = len(self.marionette.window_handles)
>  
>          try:
> -            self.assertEqual(handle_count, self._start_handle_count,
> +            #Verify the existence of leaked windows and print their URLs.

nit: This might be confusing but marionette handles tabs internally as windows. So this should be "leaked tabs".

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:28
(Diff revision 7)
>          try:
> -            self.assertEqual(handle_count, self._start_handle_count,
> -                             'A test must not leak window handles. This test started with '
> -                             '%s open top level browsing contexts, but ended with %s.' %
> -                             (self._start_handle_count, handle_count))
> +            #Verify the existence of leaked windows and print their URLs.
> +            if self._start_handle_count < handle_count:
> +                message = ('A test must not leak window handles. This test started with '
> +                           '%s open top level browsing contexts, but ended with %s.'
> +                           ' Remaining Tabs URLs:') %(self._start_handle_count, handle_count)

nit: please also a space after `%` and `,` in all cases

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:29
(Diff revision 7)
> -            self.assertEqual(handle_count, self._start_handle_count,
> -                             'A test must not leak window handles. This test started with '
> -                             '%s open top level browsing contexts, but ended with %s.' %
> -                             (self._start_handle_count, handle_count))
> +            #Verify the existence of leaked windows and print their URLs.
> +            if self._start_handle_count < handle_count:
> +                message = ('A test must not leak window handles. This test started with '
> +                           '%s open top level browsing contexts, but ended with %s.'
> +                           ' Remaining Tabs URLs:') %(self._start_handle_count, handle_count)
> +                for win in self.marionette.window_handles:

nit: I would call the variable `tab` to visualize what it really is. Maybe you can also update the variable names as set in `setUp()`.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:31
(Diff revision 7)
> -                             '%s open top level browsing contexts, but ended with %s.' %
> -                             (self._start_handle_count, handle_count))
> +                message = ('A test must not leak window handles. This test started with '
> +                           '%s open top level browsing contexts, but ended with %s.'
> +                           ' Remaining Tabs URLs:') %(self._start_handle_count, handle_count)
> +                for win in self.marionette.window_handles:
> +                    if win not in self._init_window_handles:
> +                        with self.marionette.using_context('content'):

It's enough to enter the content scope once. So move this `with` line up so that it is outside of `for`.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:32
(Diff revision 7)
> -                             (self._start_handle_count, handle_count))
> +                           '%s open top level browsing contexts, but ended with %s.'
> +                           ' Remaining Tabs URLs:') %(self._start_handle_count, handle_count)
> +                for win in self.marionette.window_handles:
> +                    if win not in self._init_window_handles:
> +                        with self.marionette.using_context('content'):
> +                            message += ' ' + self.marionette.get_url() + ','

This line would cause an exception if `get_url()` returns `None`. It's better to use a formatter instead: `' %s,' % self.marionette.get_url()`.

Personally I would prefer to build an array here first and  afterward use `','.split(array)` to create a string with comma as separator. That would eliminate the call to rstrip() too.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:34
(Diff revision 7)
> +                for win in self.marionette.window_handles:
> +                    if win not in self._init_window_handles:
> +                        with self.marionette.using_context('content'):
> +                            message += ' ' + self.marionette.get_url() + ','
> +                message2=message.rstrip(',')
> +                self.assertListEqual(self._init_window_handles,self.marionette.window_handles,message2)

This line exceeds the width of 100 columns. Please move the message into the second line.

Comment 31

[Carim Tho (:ctho)]

https://reviewboard.mozilla.org/r/40875/#review40109

Thank you for your patience and guidance. Sorry for so many mistakes while writing the patch, I've learnt a lot.
I'll update this patch, adding your suggestions.
Should I write just her name as reviewer?

> The changes in that file are most likely left overs from testing? Please make sure to check the full diff before updating the mozreview. It would safe us an additional round of reviews in the future.

It wasn't supposed to send this file. Sorry. This was just testing.

Comment 32

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Carim, yes, set maja_zf as the reviewer and I'll continue. 

Don't worry about the mistakes, you've made really good progress. :)

Comment 33

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40875/diff/7-8/

Comment 34

[Carim Tho (:ctho)]

Hi, I just saw that I forgot the space before and after the '=' on line 22, sorry. I'll submit again with the space added.

Comment 35

[Carim Tho (:ctho)]

Comment on attachment 8731844 [details]
MozReview Request: Bug 1146990 - Log the URL of each tab when test leaks window handles; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40875/diff/8-9/

Comment 36

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Carim, could you post an example of what the log looks like with your patch when there are leftover tabs?

Comment 37

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Could you also go through the list of 13 issues from Henrik in Mozreview and mark them as 'resolved' or reply to them as appropriate?

Comment 38

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/40875/#review40377

This looks good to me, I just need you to answer the questions I posted in my latest Bugzilla comments before I actually land your patch. :)

Comment 39

[Carim Tho (:ctho)]

ok. I'll post it and I'll mark them as resolved and reply them.

Comment 40

[Carim Tho (:ctho)]

I replied some of them, but I think I forgot to publish it, I'll publish the answers.

Comment 41

[Carim Tho (:ctho)]

https://reviewboard.mozilla.org/r/40875/#review38105

> Why don't we have a simple `if` condition here which checks for the number of open handles? With that we would not need the `except` block.
> 
> Also shouldn't the message with the list of tabs be better part of the assertion? A separate logging error doesn't seem to be the best fit for me.

I was having trouble in merging the URL message together wuth the assertEqual command, that's one of the reason I thought about the except block, but I can try changing the assertion command to include the URLs and delete the except block.

> How would that look like in the log output? I wonder if we should better concanate tab URLs with comma.

Actually, the output looks fine for me, each URL is printed on separate lines.

Comment 42

[Carim Tho (:ctho)]

Attachment: LOG of the patch, when there are remaining tabs.

Hi! This is the LOG of the patch, when there are remaining tabs, showing the AssertionError message.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1354379685b5

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1354379685b5

Comment 45

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

(In reply to Carim Tho (:ctho) from comment #42)
> Hi! This is the LOG of the patch, when there are remaining tabs, showing the
> AssertionError message.

That looks great! Thanks a lot for working on this bug and getting your code landed!

Comment 46

[Carim Tho (:ctho)]

(In reply to Henrik Skupin (:whimboo) from comment #45)
> (In reply to Carim Tho (:ctho) from comment #42)
> > Hi! This is the LOG of the patch, when there are remaining tabs, showing the
> > AssertionError message.
> 
> That looks great! Thanks a lot for working on this bug and getting your code
> landed!

(In reply to Maja Frydrychowicz (:maja_zf) from comment #38)
> https://reviewboard.mozilla.org/r/40875/#review40377
> This looks good to me, I just need you to answer the questions I posted in
> my latest Bugzilla comments before I actually land your patch. :)

Hi! Thanks for your guidance, I've learned a lot through this experience.