725094 - Add a try/except around Robocop tests to catch test errors vs Fennec errors

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase

The generated testcase throws a "INSTRUMENTATION_RESULT: shortMsg=Process crashed.
INSTRUMENTATION_CODE: 0" message every time it is run.

While the testcase may probably be invalid, it shouldn't cause a crash.

On m-c changeset feb866aec8d8, tested on Mac OS X connected to a Galaxy Tab 10.1.

My make command is:

make -C <objdir> mochitest-robotium

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Please add [testFuzzer] to robocop.ini, in any case my robocop.ini looks like:

# Make sure main Robocop stuff is not broken.
[testNewTab]
[testFuzzer]
#[testAwesomebar]
#[testBookmark]
#[testLoad]
#[testNewTab]
#[testPanCorrectness]

# Used for Talos, please don't use in mochitest
#[testPan]
#[testCheck]

I usually first run testNewTab to ensure the application & framework are working properly before running testFuzzer.

Comment 2

[Geoff Brown [:gbrown]]

I reproduced the crash. Here's the adb logcat:

I/GeckoApp(  314): URI - http://www.cnn.com/, title - CNN.com - Breaking News, U.S., World, Weather, Entertainment & Video News
D/Robocop (  314): Waking up on handleMessage
D/Robocop (  314): received event DOMContentLoaded
D/Robocop (  314): unblocked on expecter for DOMContentLoaded
E/GeckoAppShell(  314): top level exception
E/GeckoAppShell(  314): org.mozilla.fennec_mozdev.RoboCopException: click: unable to find view 2131492918
E/GeckoAppShell(  314): 	at org.mozilla.fennec_mozdev.FennecNativeElement$1.run(FennecNativeElement.java:84)
E/GeckoAppShell(  314): 	at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoAppShell(  314): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoAppShell(  314): 	at android.os.Looper.loop(Looper.java:123)
E/GeckoAppShell(  314): 	at org.mozilla.gecko.GeckoApp$32.run(GeckoApp.java:1777)
E/GeckoAppShell(  314): 	at android.os.Handler.handleCallback(Handler.java:587)
E/GeckoAppShell(  314): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoAppShell(  314): 	at android.os.Looper.loop(Looper.java:123)
E/GeckoAppShell(  314): 	at android.app.ActivityThread.main(ActivityThread.java:4627)
E/GeckoAppShell(  314): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell(  314): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/GeckoAppShell(  314): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:858)
E/GeckoAppShell(  314): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
E/GeckoAppShell(  314): 	at dalvik.system.NativeStart.main(Native Method)

Comment 3

[Geoff Brown [:gbrown]]

This seems to be the offending code:

+        Element rld = mDriver.findElement(getActivity(), "reload");
+        Activity activityRld = getActivityFromClick(rld);

I believe "reload" identifies the "Reload" menu item, which is not displayed when getActivityFromClick is called. To select that menu item, you need to bring up the menu and then click on the Reload menu item. There is some code in this patch that uses menu items: https://bugzilla.mozilla.org/attachment.cgi?id=594772 (from bug 720930).

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: more verbose logcat output

Just to get more context about what happens before and after the crash.

I had previously run `cat logcatOutput.txt | grep 'Gecko\|Robocop'` so only lines starting with Gecko or Robocop are shown.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> I believe "reload" identifies the "Reload" menu item, which is not displayed
> when getActivityFromClick is called.

Crashing because the menu item is not displayed or is not available, seems weird. Shouldn't it output some form of error message instead? (e.g. MENUITEM_NOT_FOUND)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Robocop is an early stage tool.  I can see as time goes on we can have a more complete API and error handling infrastructure, There will be some things which are very complex to handle because Robocop hooks into the Instrumentation activity of a program, in these cases the test cases need to be handling the logic.  

I would advocate making the test cases stronger and the more examples we have of robust test cases the better our chances of future tests doing similar techniques.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I spoke w/ jmaher on IRC. Ideally, there should be some way to differentiate Fennec crashes vs Robocop framework crashes.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

I looked into adding something around the test block and it didn't seem straightforward, there could be something I am missing though.

My goal was to not require a try/except written in every test case.  My goal was to find a place in BaseTest or some of the robocop utilities that could wrap it.

Comment 9

[Geoff Brown [:gbrown]]

The specific issue that started this bug was that Robocop code intentionally threw an exception because a requested view was not available. We only do that in a few places, but some of those exceptions are easily encountered, at least when writing new tests. Perhaps we should convert our throw's to boolean return values. This wouldn't catch exceptions thrown by robotium code itself, or other java components that robocop is using, but I suspect it would greatly reduce the chance of robocop code throwing exceptions. Thoughts?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

I think catch or not throwing exceptions in our robocop code is good.  The main reason for that is we can report errors back to buildbot easier that way, it also helps us find common errors.

Comment 11

[Geoff Brown [:gbrown]]

Attachment: wip patch

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 624407 [details] [diff] [review]
wip patch

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

hey, this is a nice looking patch already.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 624407 [details] [diff] [review]
> wip patch
> 
> Review of attachment 624407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hey, this is a nice looking patch already.

r+ on a wip patch? So not really ready for landing, I suppose..

Comment 14

[Geoff Brown [:gbrown]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #13)
> r+ on a wip patch? So not really ready for landing, I suppose..

I'm just trying to finish off some testing...hope to check in soon!

Comment 15

[Geoff Brown [:gbrown]]

Attachment: previous patch updated for bitrot

r=jmaher carried

Comment 16

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c66ed8fea9

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d3c66ed8fea9