775071 - testCheck3 doesn't actually disable screenshotting

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

testCheck3 looks for the disableScreenshot function on GeckoAppShell when in fact it is on ScreenshotHandler. Therefore it never actually disables the screenshot code, and testCheck3 ends up being the same as testCheck2.

This was never caught because the mAsserter instance in talos tests is a sad empty shell that does nothing. That should probably be changed so that it either throws an UnsupportedOperationException, or does something useful rather than silently doing nothing.

Comment 1

[Geoff Brown [:gbrown]]

It looks like disableScreenshot() was moved by bug 755070, so testCheck3 results before June 4 should be okay...but it was probably disabled for much of that time!

I have been frustrated and concerned by the Talos asserter before -- we really should do something about that.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Filed bug 775102 for fixing FennecTalosAssert

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Make the test fail

Untested, but this corrects the error detection to make the test actually fail as it should.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Make the test pass

Also untested but this should fix the underlying bug in the test to make it pass again.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Make the test fail

This aborts the test if we can't call disableScreenshot(), which prevents the __start_report output from being printed, and in combination with bug 775102, causes the test to fail. Try run at https://tbpl.mozilla.org/?tree=Try&rev=72c8fbfe2a71

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Make the test pass

This fixes the method-finding code so the test passes again. Try run at https://tbpl.mozilla.org/?tree=Try&rev=4bf4eee5ce53 (first one had an infra error during cleanup but from the log it's a pass).

Note that there's still this error in the log:

NOISE: __FAILchecking that painted surface loaded: painted surface loaded__FAIL

which should be investigated.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

And filed bug 775614 for the painted surface failure in the previous comment.

Comment 8

[Geoff Brown [:gbrown]]

Comment on attachment 643888 [details] [diff] [review]
(1/2) Make the test fail

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

::: mobile/android/base/tests/testCheck3.java.in
@@ +16,5 @@
>          // this distinguishes this test from testCheck2, which is otherwise
>          // identical.
> +        if (!disableScreenshot()) {
> +            // if this fails the running the test is pointless. abort,
> +            // the lack of report output should get picked up as an error

nit -- this comment is a little confusing. Maybe something like "If the screenshot cannot be disabled, there is no point to this test: abort..." ?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Landed with comment fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6a10a234fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54f24e6d07c

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d6a10a234fd
https://hg.mozilla.org/mozilla-central/rev/b54f24e6d07c