1052211 - Add --autoclose option to make mochitest single test run close the browser after the test has completed

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

I did:
./mach mochitest-plain content/media/webspeech/recognition/test/test_call_start_from_end_handler.html

Afterwards, the browser should close, but currently, it doesn't do that anymore.
I noticed that the url in the browser doesn't have closewhendone=true anymore.
This is the url showing in the browser:
http://mochi.test:8888/tests/?autorun=1&consoleLevel=INFO&manifestFile=tests.json&failureFile=/Users/mwargers/mozilla-central/obj-x86_64-apple-darwin13.3.0/.mozbuild/mochitest_failures.json&dumpOutputDirectory=%2Fvar%2Ffolders%2F_z%2Fchnvlqgn4hn4vpqkdk5gkxk00000gn%2FT

could this be a regression from bug 1014062?

Comment 1

[:Gijs (out for now; he/him)]

Err, I didn't think it exited before my changes, either? See bug 1014062 comment 5 and later discussion on that point. Keeping that behaviour was intentional.

I just checked on my Linux VM which had an fx-team tree on 195784:d5e892913147, which was before bug 1014062 landed, and it also doesn't exit the browser when executing (I mean, I got bored of waiting after the test had clearly stopped for more than 10s, but yeah...)

./mach mochitest-plain content/media/webspeech/recognition/test/test_call_start_from_end_handler.html

so I'm tempted to mark this WFM/INVALID.

Comment 2

[(not currently active) Ted Mielczarek]

This is the intended behavior, yes. We should probably add the inverse of  "--keep-open" to support this use case.

Martijn: the intent here is that people running one test are usually developing the test, so they want the easy edit-and-reload cycle instead of having to relaunch the browser.

Comment 3

[Martijn Wargers (dead)]

Fine by me, I was looking for a leak in a mochitest and for that, the browser has to close.
I also got confused by ./mach mochitest --help, where I only see the --keep-open option, which led me to believe it would normally close.

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> This is the intended behavior, yes. We should probably add the inverse of 
> "--keep-open" to support this use case.

Morphing per this instead.

Comment 5

[Martijn Wargers (dead)]

Attachment: 1052211.diff

Something like this seems to work.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8475516 [details] [diff] [review]
1052211.diff

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

::: testing/mochitest/mach_commands.py
@@ +210,5 @@
>          shuffle is whether test order should be shuffled (defaults to false).
>  
>          keep_open denotes whether to keep the browser open after tests
>          complete.
> +        

nit: whitespace

@@ +288,5 @@
>          if dmd:
>              options.dmdPath = self.bin_dir
>  
>          options.autorun = not no_autorun
> +        options.closeWhenDone = not keep_open or autoclose

Isn't this going to wind up always being true unless callers pass in both arguments? The defaults for these are False and True, so this statement is True unless you pass in keep_open=True and autoclose=False.

Comment 8

[:Gijs (out for now; he/him)]

Attachment: fix default closure behaviour for mochitests and add --auto-close option to force closing the browser,

Stealing this. This restores the previous default behaviour (I think?) and allows you to override this by either closing or keeping open, with a flag.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8487927 [details] [diff] [review]
fix default closure behaviour for mochitests and add --auto-close option to force closing the browser,

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

driveby :)

::: testing/mochitest/mach_commands.py
@@ +291,5 @@
>          if dmd:
>              options.dmdPath = self.bin_dir
>  
>          options.autorun = not no_autorun
> +        options.closeWhenDone = False if closure_behaviour == 1 else True

maybe closeWhenDone = closure_behaviour != 1

@@ +433,3 @@
>      func = keep_open(func)
>  
> +    autoclose = CommandArgument('--autoclose', action='store_const',

bikeshed: I wonder if --auto-close is more consistent?

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8487927 [details] [diff] [review]
fix default closure behaviour for mochitests and add --auto-close option to force closing the browser,

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

::: testing/mochitest/mach_commands.py
@@ +216,5 @@
> +        complete:
> +        0 for automatic (closing after multiple tests, or a single
> +        mochitest-non-plain, staying open for a single mochitest-plain test),
> +        1 for keeping the browser open,
> +        2 for closing it.

I'd rather this was a string (wish Python had a built-in enum type).

Comment 11

[:Gijs (out for now; he/him)]

Attachment: fix default closure behaviour for mochitests and add --auto-close option to force closing the browser,

Now with strings and auto-close instead of autoclose (just checking that this is what you meant, Ted...).

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8495197 [details] [diff] [review]
fix default closure behaviour for mochitests and add --auto-close option to force closing the browser,

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

Yes, this looks better, thanks!

::: testing/mochitest/mach_commands.py
@@ +186,5 @@
>          options.xrePath = xre_path
>          return mochitest.run_remote_mochitests(parser, options)
>  
>      def run_desktop_test(self, context, suite=None, test_paths=None, debugger=None,
> +        debugger_args=None, slowscript=False, screenshot_on_fail = False, shuffle=False, closure_behaviour="auto",

nit: here and elsewhere, single quotes for strings

Comment 13

[:Gijs (out for now; he/him)]

w/ single quotes,

remote:   https://hg.mozilla.org/integration/fx-team/rev/437731e3c704

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/437731e3c704