Closed Bug 1052211 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: martijn.martijn, Assigned: Gijs)

References

Details

Attachments

(2 files, 1 obsolete file)

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?
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.
Blocks: 1014062
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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
(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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: ./mach mochitest-plain single test run doesn't close the browser anymore afterwards → Add --autoclose option to make mochitest single test run close the browser after the test has completed
Attached patch 1052211.diffSplinter Review
Something like this seems to work.
Attachment #8475516 - Flags: review?(ted)
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.
Attachment #8475516 - Flags: review?(ted)
Duplicate of this bug: 1065776
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.
Attachment #8487927 - Flags: review?(ted)
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
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?
Attachment #8487927 - Flags: feedback+
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).
Attachment #8487927 - Flags: review?(ted) → review+
Now with strings and auto-close instead of autoclose (just checking that this is what you meant, Ted...).
Attachment #8495197 - Flags: review?(ted)
Attachment #8487927 - Attachment is obsolete: true
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
Attachment #8495197 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/437731e3c704
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.