Closed Bug 991348 Opened 10 years ago Closed 10 years ago

Let mochitests take screenshots for test failures

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file, 4 obsolete files)

Sometimes while running tests it is useful to have screenshots. Unfortunately, it's not always possible to quickly take screenshots when tests are running.


The proposal here is to add a screenshot() method that takes a chrome window as an argument and generates a screenshot of it. We can either save this in a predefined folder, or output it to the log as a data URI.

This is not intended to be used in final tests that get checked in.
My use case is that I'm running into a test that fails when running on the remote test machine, but not locally - so I want to see what state the browser is in at the point of failure.

It might even be better to have it so that the mochitest framework takes these screenshots at the point of test failure, instead of making mochitests themselves do it. It looks like there's currently support for that sort of thing for test timeouts[1], but not general test failures, from what I can see.

[1]: Search for "dumpScreenOnTimeout" in testing/mochitest/runtests.py
Summary: Allow for the creation of screenshots from mochitests → Let mochitests take screenshots for test failures
After a discussion in IRC, I've changed the bug to focus on the use case mentioned above.

Basically, one should be able to ask the mochitests to take screenshots on all test failures. This would be off by default, and can be turned on as a mach argument.

One major use for this is to take screenshots when testing on try. I'm not entirely sure how to do this, though. jmaher, could you provide some insight here? Thanks.
Attached patch WIP patch (obsolete) — Splinter Review
This is a simple patch with no command line control. The screenshot-on-fail feature is by default turned off.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Oops, forgot to needinfo.

As far as try goes IMO the way to do this would be to add an extra try flag that enables the corresponding mochitest flag.
Flags: needinfo?(jmaher)
Comment on attachment 8401294 [details] [diff] [review]
WIP patch

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

I would like this to include a method for triggering this, even if it is a commandline option that we can later hook into try server.  Also I saw you needinfo? jmaher, but the email was a different jmaher- feel free to clear it if you were looking for me.

Thanks for putting this patch together so fast!

::: testing/mochitest/runtests.py
@@ +1318,5 @@
>        """format the line"""
>        return line.rstrip().decode("UTF-8", "ignore")
>  
>      def dumpScreenOnTimeout(self, line):
> +      if not self.dump_screen_on_fail and self.dump_screen_on_timeout and "TEST-UNEXPECTED-FAIL" in line and "Test timed out" in line:

I don't understand this, why do we need 'not self.dump_screen_on_fail' ?
Attachment #8401294 - Flags: review-
Oops.

Yes, the plan was a command line option, which I'm working on now.

`not self.dump_screen_on_fail` is to avoid duplication of screenshots. That can be removed, I guess (looks like dumpScreen already handles this)
Flags: needinfo?(jmaher)
thanks, I just want to make sure we still dumpscreen for timeouts
Attached patch Patch 1 (obsolete) — Splinter Review
This patch seems to work. It adds the `--screenshot-on-fail` command line switch which makes it create screenshots on test failures.

Note that for this to work the `$MOZ_UPLOAD_DIR` environment variable has to be set to the absolute path of the folder where you want to store the screenshots.

Also, I just realized that this may not always take the correct screenshot. The Python is asynchronous wrt the javascript, so when a test fails the test will continue to run while runtests.py is trying to get a screenshot.
Attachment #8401294 - Attachment is obsolete: true
Attachment #8401523 - Flags: review?(jmaher)
Comment on attachment 8401523 [details] [diff] [review]
Patch 1

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

Is this intended to be just for developers?  I was under the impression we might want this on tbpl, and if so, we would need an option in mochitest_options.py.

As for the timing of python vs javascript, I am not sure how to solve that.  I can see that as being a real problem, but something that we wouldn't need to solve in this bug.

::: testing/mochitest/runtests.py
@@ +1321,5 @@
>        """format the line"""
>        return line.rstrip().decode("UTF-8", "ignore")
>  
>      def dumpScreenOnTimeout(self, line):
> +      if not self.dump_screen_on_fail and self.dump_screen_on_timeout and "TEST-UNEXPECTED-FAIL" in line and "Test timed out" in line:

let me get this clear, if we get a timeout and dump-screen_on_fail=True, then we will print the screenshot in the dumpScreenOnFail() method below?
Attachment #8401523 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8401523 [details] [diff] [review]
> Is this intended to be just for developers?  I was under the impression we
> might want this on tbpl, and if so, we would need an option in
> mochitest_options.py.


There already is an option in mach_commands.py. I'm not too familiar with mochitest, so I don't exactly know how it can be called independently. Will adding the option to the mochitest_options array be fine? Or will I have to make other changes?

Yes, this is intended to eventually be callable via tbpl.


> let me get this clear, if we get a timeout and dump-screen_on_fail=True,
> then we will print the screenshot in the dumpScreenOnFail() method below?

Yep.
If you want, we can file a followup bug to integrate the option into the harness, which could then be used in production.

As for making the option work, you would need to add an option to mochitest_options.py, then make sure you can get the option passed down to runApp() inside of runtests.py.
I added the switch to mochitest_options.

I'm not sure how to test if this works, though. I am able to test this via `./mach mochitest-browser --screenshot-on-fail ...`, but that takes the switch from mach_commands. How do I test switches added to mochitest_options?
Attachment #8401523 - Attachment is obsolete: true
Attachment #8402019 - Flags: review?(jmaher)
Comment on attachment 8402019 [details] [diff] [review]
Patch 2 : added mochitest command

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

this looks to be the right way to do this.  As for testing this, you would need to run this outside of mach and of course make sure this doesn't break anything on try server.

Here is the closest way to test to what is on try server:
cd objdir
make package-tests
cd dist/test-package-stage
python mochitest/runtests.py --app ../bin/firefox --xre-path ../bin --certificate-path certs --utility-path bin --autorun --close-when-done --console-level=INFO --extra-profile-file bin/plugins --test-path <path to a failing test> --screenshot-on-fail
Attachment #8402019 - Flags: review?(jmaher) → review+
Ah, thanks for the testing instructions :)

I'll have a look tomorrow then, bit busy today.
Uh, exactly how do I give the path? In my case, I've put an `ok(false)` in `browser/components/preferences/tests/browser_connection_bug388287.js`. It doesn't seem to open it (it's looking for it in ./tests, whereas the file is in ../browser/browser. Explicitly giving the path of the test just opens it in firefox as a text file.

How should I test this?
Flags: needinfo?(jmaher)
Attached patch Patch 3 : fix error (obsolete) — Splinter Review
Fixing an error in the mochitest options. Haven't been able to test it properly yet, though.
Attachment #8402019 - Attachment is obsolete: true
ok, if you are doing it from the test-package (or .zip file), you will do this:
python mochitest/runtests.py --app ../bin/firefox --xre-path ../bin --certificate-path certs --utility-path bin --autorun --close-when-done --console-level=INFO --extra-profile-file bin/plugins --test-path browser/components/preferences/tests/browser_connection_bug388287.js --browser-chrome --screenshot-on-fail

^ note the --browser-chrome added.
Flags: needinfo?(jmaher)
Comment on attachment 8403000 [details] [diff] [review]
Patch 3 : fix error

I tested it, this works.

It also takes screenshots of timeouts (once), which is good.
Attachment #8403000 - Flags: review?(jmaher)
Comment on attachment 8403000 [details] [diff] [review]
Patch 3 : fix error

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

thanks for putting this together!
Attachment #8403000 - Flags: review?(jmaher) → review+
You're welcome :)

Note: While playing with it a bit I realized that it's not so useful for browser-chrome, since browser-chrome tests usually manipulate the DOM a lot before rendering can even happen. (It will work if you explicitly ask it to wait at the failure point). It seems to give good screenshots for normal mochitests.

How do we go about making this a tryserver parameter?
Blocks: 995427
Pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=619164f34471

As far as I can tell this current version will just ignore the flag on Android. I don't know if we need the capability of taking screenshots for Android (it already automatically does so on timeout, but in a different way), and I don't have a way of testing this.
Attachment #8403000 - Attachment is obsolete: true
Comment on attachment 8405752 [details] [diff] [review]
Patch 4: Make it not break android

Alright, the try build seems to have passed. I put a more extensive one here: https://tbpl.mozilla.org/?tree=Try&rev=c1f2058d24fc (if you don't think that we need that, feel free to cancel, since a mozilla-inbound/fx-team push will trigger the same build anyway)
Attachment #8405752 - Flags: review?(jmaher)
Ok, this patch is based on a rather old m-c (A weekish?), and mochitest-dt requires the --subsuite option (and breaks[1] without it)

I'll rebase it to the current one so that dt doesn't break.

 [1]: https://tbpl.mozilla.org/?tree=Try&rev=c1f2058d24fc
Comment on attachment 8405752 [details] [diff] [review]
Patch 4: Make it not break android

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

any concerns with b2g?
Attachment #8405752 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #27)
> Comment on attachment 8405752 [details] [diff] [review]
> Patch 4: Make it not break android
> any concerns with b2g?

I don't think so. All the test failures are covered by multiple intermittent-orange bugs, so that seems to be expected.

Marking as checkin-needed, I don't think there are any further issues.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6394c88d9f90
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: