933842 - Add ability to specify test root in SUTAgent.ini

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Description

[Bob Clary [:bc] (inactive)]

The heuristics for setting the test root can sometimes choose a root which is not appropriate for a specific device. This can result in a test beginning to run only to fail later when the inappropriate path is actually used.

It would be helpful in those cases where we know that a specific test root should be used on a device to be able to specify it in the SUTAgent.ini file for the device.

If the specified test root is not available, the SUTAgent should fail visibly and not soft fail to an unsupported location.

As part of this bug, I intend to make the automatic selection of the test root less prone to timing issues. I will also add the SUTAgent version, the testroot to the SUTAgent status display.

Comment 1

[Geoff Brown [:gbrown]]

Recall that certain test harnesses also have an option to over-ride the devicemanager test root. For example, https://hg.mozilla.org/mozilla-central/file/b199e59e7f6d/testing/mochitest/runtestsremote.py#l128. I don't think that is a complicating factor for this bug, but thought I would mention it in case that is of use, or concern. I suspect --remoteTestRoot is a convenient way for setting the test root for a developer doing ad-hoc testing on a local device, while a SUTAgent.ini setting would be a better solution for cases like phonedash.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: bug-933842-sutagent-testroot.patch

Candidates for test root are checked for writability rather than the mixture of Environment.getExternalStorageState() or the ability to create a file.

Once determined, the test root is cached. This speeds things up a tiny bit but also eliminates the possibility the test root might change while the SUTAgent is running.

If the SUTAgent.ini files contains a [Device] section with property TestRoot, it is used as the test root if it is writable. It is an error if it is specified and is not writable during the SUTAgent's onCreate method.

If the SUTAgent.ini file does not contain a specification for the TestRoot, the test root is determined by the first writable instance of the external storage directory or the  /data/local directory.

If no writable TestRoot is found, the value '##AGENT-WARNING##  unable to determine test root' is cached as its value.

The values for the SUTAgent's version and the value of TestRoot are displayed on the device's screen and written to the log. Thus this bug also fixes bug 933855.

I've tested with and without the [Device] section set to /mnt/sdcard and with writable and read only /mnt/sdcard on a device where the external storage is the external sdcard. To test with a read only external sdcard do:

mount -o ro,remount /dev/block/vold/179:1 /mnt/sdcard

where the /dev/block/... is the appropriate path for the sdcard on your device.

Comment 3

[Bob Clary [:bc] (inactive)]

PS. I don't get the indentation scheme in these files, so if they are off base please enlighten me. ;-)

Comment 4

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

Comment on attachment 826632 [details] [diff] [review]
bug-933842-sutagent-testroot.patch

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

the indentation is a relic from the original version of this.  We could reformat it if you want to :)

My Java is rusty, but this general logic looks good.

::: build/mobile/sutagent/android/DoCommand.java
@@ +106,5 @@
>  
>      String ffxProvider = "org.mozilla.ffxcp";
>      String fenProvider = "org.mozilla.fencp";
>  
> +    public final String prgVersion = "SUTAgentAndroid Version 1.19";

would we want to change this to 1.20?

@@ +1390,5 @@
> +
> +    public void SetTestRoot(String testroot)
> +        {
> +        Boolean success = false;
> +        if (testroot != "") {

I believe it should be !testroot.equals("")

::: build/mobile/sutagent/android/SUTAgentAndroid.java
@@ +66,5 @@
>      public static String sPowerStatus = null;
>      public static int    nChargeLevel = 0;
>      public static int    nBatteryTemp = 0;
>      public static long   nCreateTimeMillis = System.currentTimeMillis();
> +    public static String TestRoot = "";

the standard in this file would make: sTestRoot

@@ +162,5 @@
>          fixScreenOrientation();
>  
>          DoCommand dc = new DoCommand(getApplication());
>  
> +        Log.i("SUTAgentAndroid", "INFO: " + dc.prgVersion);

"INFO: SUTAgent Version: "

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 826632 [details] [diff] [review]
> bug-933842-sutagent-testroot.patch
> 
> Review of attachment 826632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the indentation is a relic from the original version of this.  We could
> reformat it if you want to :)
> 
> My Java is rusty, but this general logic looks good.
> 
> ::: build/mobile/sutagent/android/DoCommand.java
> @@ +106,5 @@
> >  
> >      String ffxProvider = "org.mozilla.ffxcp";
> >      String fenProvider = "org.mozilla.fencp";
> >  
> > +    public final String prgVersion = "SUTAgentAndroid Version 1.19";
> 
> would we want to change this to 1.20?
> 

Either here or in Bug 934012. Or do you mean change it to

public final String prgVersion = "1.20";

and change the output to include the "SUTAgent Version:" ?
	
> @@ +1390,5 @@
> > +
> > +    public void SetTestRoot(String testroot)
> > +        {
> > +        Boolean success = false;
> > +        if (testroot != "") {
> 
> I believe it should be !testroot.equals("")

Ok. This worked in testing but I can imagine this might cause problems.

> 
> ::: build/mobile/sutagent/android/SUTAgentAndroid.java
> @@ +66,5 @@
> >      public static String sPowerStatus = null;
> >      public static int    nChargeLevel = 0;
> >      public static int    nBatteryTemp = 0;
> >      public static long   nCreateTimeMillis = System.currentTimeMillis();
> > +    public static String TestRoot = "";
> 
> the standard in this file would make: sTestRoot

Ok.

> 
> @@ +162,5 @@
> >          fixScreenOrientation();
> >  
> >          DoCommand dc = new DoCommand(getApplication());
> >  
> > +        Log.i("SUTAgentAndroid", "INFO: " + dc.prgVersion);
> 
> "INFO: SUTAgent Version: "

With the current value of prgVersion, this already displays:

11-03 23:36:03.057  2078  2078 I SUTAgentAndroid: INFO: SUTAgentAndroid Version 1.19

but if we change prgVersion to just be "1.20" we would need to do this.

Comment 6

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

ok, I would maybe have this be "INFO: Version: "

Comment 7

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

Please ignore my comments around changing the version string.  It should remain the same and in this bug or another we should s/1.19/1.20/.

there are parsers that look for this version in sut_tools, we shouldn't be breaking them unless we need to.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: bug-933842-sutagent-testroot.patch v2

for comparison sake.

Comment 9

[Bob Clary [:bc] (inactive)]

Callek,

The matching SUTAgentAndroid and watcher are at:

http://people.mozilla.org/~bclary/sutAgentAndroid.apk
http://people.mozilla.org/~bclary/Watcher.apk

What would be the procedure for testing these to make sure I haven't regressed anything on tegras and pandas?

/bc

Comment 10

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

My understanding is Callek or pmoore will run these in the staging instance; is there an updated watcher as well?  That would increase the deployment and testing costs a bit.

this usually take a couple days.

Comment 11

[Bob Clary [:bc] (inactive)]

I haven't changed the watcher, but included a link to the one that was built along side of the SUTAgent. See comment 9.

If we don't need to test the watcher, I'm fine with that. I didn't make any changes to it. Not sure what interactions it might have but I don't have a good feel for that.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 826632 [details] [diff] [review]
bug-933842-sutagent-testroot.patch

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

::: build/mobile/sutagent/android/DoCommand.java
@@ +1375,5 @@
>          }
>  
> +    private Boolean _SetTestRoot(String testroot)
> +        {
> +        String isWritable = IsDirWritable(testroot);

It looks to me like IsDirWritable only checks for directory existence - isDirectory(). I don't think it actually checks for write permission (File.canWrite(), for instance).

@@ +1390,5 @@
> +
> +    public void SetTestRoot(String testroot)
> +        {
> +        Boolean success = false;
> +        if (testroot != "") {

Yes, use .equals for string comparisons please.

Consider checking for null too.

@@ +1417,3 @@
>      public String GetTestRoot()
>          {
> +        if (SUTAgentAndroid.TestRoot == "") {

.equals()

::: build/mobile/sutagent/android/SUTAgentAndroid.java
@@ +162,5 @@
>          fixScreenOrientation();
>  
>          DoCommand dc = new DoCommand(getApplication());
>  
> +        Log.i("SUTAgentAndroid", "INFO: " + dc.prgVersion);

"INFO" is redundant; Log.i marks the entry as info...although it will only display as /I.

I suggest:

Log.i("SUTAgentAndroid", dc.prgVersion);

which should produce something like:

I/SUTAgentAndroid(19441): SUTAgentAndroid Version 1.19

Comment 13

[Mark Côté [:mcote]]

(In reply to Joel Maher (:jmaher) from comment #7)
> Please ignore my comments around changing the version string.  It should
> remain the same and in this bug or another we should s/1.19/1.20/.
> 
> there are parsers that look for this version in sut_tools, we shouldn't be
> breaking them unless we need to.

Seconding this, as mozbase's mozdevice relies on this string as well.

Comment 14

[Geoff Brown [:gbrown]]

Comment on attachment 826777 [details] [diff] [review]
bug-933842-sutagent-testroot.patch v2

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

See my previous comments...some still apply!

Comment 15

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #12)
> Comment on attachment 826632 [details] [diff] [review]
> bug-933842-sutagent-testroot.patch
> 
> Review of attachment 826632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/mobile/sutagent/android/DoCommand.java
> @@ +1375,5 @@
> >          }
> >  
> > +    private Boolean _SetTestRoot(String testroot)
> > +        {
> > +        String isWritable = IsDirWritable(testroot);
> 
> It looks to me like IsDirWritable only checks for directory existence -
> isDirectory(). I don't think it actually checks for write permission
> (File.canWrite(), for instance).
> 

I tested by making /mnt/sdcard read only and it detected the inability to write to the sdcard pretty well.

See http://mxr.mozilla.org/mozilla-central/source/build/mobile/sutagent/android/DoCommand.java#1990

   sRet = "[" + sTmpDir + "] " + (dir.canWrite() ? "is" : "is not") + " writable";

which is the relevant branch for testing for writable testroots.

> @@ +1390,5 @@
> > +
> > +    public void SetTestRoot(String testroot)
> > +        {
> > +        Boolean success = false;
> > +        if (testroot != "") {
> 
> Yes, use .equals for string comparisons please.
> 

Done in v2.

> Consider checking for null too.
> 

It is always non null due to attempting to load it from the ini file. If the setting doesn't exist, it is set to the empty string.

> @@ +1417,3 @@
> >      public String GetTestRoot()
> >          {
> > +        if (SUTAgentAndroid.TestRoot == "") {
> 
> .equals()
> 

Done in v2.

> ::: build/mobile/sutagent/android/SUTAgentAndroid.java
> @@ +162,5 @@
> >          fixScreenOrientation();
> >  
> >          DoCommand dc = new DoCommand(getApplication());
> >  
> > +        Log.i("SUTAgentAndroid", "INFO: " + dc.prgVersion);
> 
> "INFO" is redundant; Log.i marks the entry as info...although it will only
> display as /I.
> 
> I suggest:
> 
> Log.i("SUTAgentAndroid", dc.prgVersion);
> 
> which should produce something like:
> 
> I/SUTAgentAndroid(19441): SUTAgentAndroid Version 1.19

ok.

Comment 16

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #15)
> (In reply to Geoff Brown [:gbrown] from comment #12)
> > It looks to me like IsDirWritable only checks for directory existence -
> > isDirectory(). I don't think it actually checks for write permission
> > (File.canWrite(), for instance).
> > 
> 
> I tested by making /mnt/sdcard read only and it detected the inability to
> write to the sdcard pretty well.
> 
> See
> http://mxr.mozilla.org/mozilla-central/source/build/mobile/sutagent/android/
> DoCommand.java#1990
> 
>    sRet = "[" + sTmpDir + "] " + (dir.canWrite() ? "is" : "is not") + "
> writable";
> 
> which is the relevant branch for testing for writable testroots.

I see now -- I missed that before.

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: bug-933842-sutagent-testroot.patch v3

patch v3 with the latest changes
I also updated http://people.mozilla.org/~bclary/sutAgentAndroid.apk

Comment 18

[Bob Clary [:bc] (inactive)]

Callek, please hold off testing for the moment. I'm going to build with this patch, bug 933501 and bug 934012 and repost the apk. That way we can test all of the changes going in. I'll ping here when it is ready.

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: bug-933842-sutagent-testroot.patch v3

I had a conflict when updating my tree, so I've updated the v3 patch again.

I've pushed the new apk to http://people.mozilla.org/~bclary/sutAgentAndroid.apk

Comment 20

[Justin Wood (:Callek)]

Ok, after just speaking to bc on IRC, salient points:

* Using /data/local as testroot can possibly bust some devices in unpredictable ways
** (A way that has happened in the past with large xpcshell test uses was com.android.phone crashing and breaking other tests, took days of research to figure it out once we started to see the pattern)

* Newer android devices are starting to make getExternalStorage point at an internal sdcard rather than a user-inserted SDCard
** This means we want to keep the ability to override testroot

* Nothing in releng expects or wants /data/local as the testroot, so eliminating the fallback is a good idea.

:bc will do just that, eliminate the fallback logic but leave the overrideable ini behavior in.

At which point [after code review] I will test the SUTAgent.apk in staging

Comment 21

[Bob Clary [:bc] (inactive)]

jgriffin, ctalbert: If you have any objections, please speak up.

Comment 22

[Jonathan Griffin (:jgriffin)]

(In reply to Bob Clary [:bc:] from comment #21)
> jgriffin, ctalbert: If you have any objections, please speak up.

None here.

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: bug-933842-sutagent-testroot.patch

This is v3 with the /data/local fall back removed. It should be easy to compare against the previous patches.

Comment 24

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

Comment on attachment 828777 [details] [diff] [review]
bug-933842-sutagent-testroot.patch

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

thanks bc for working on getting sutagent upgraded!

Comment 25

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d562b336731

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d562b336731