Closed Bug 852264 Opened 8 years ago Closed 7 years ago

[s1s2] don't hardcode URLs for local test files

Categories

(Testing :: Autophone, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Assigned: bc)

Details

Attachments

(3 files)

Our tests use file://mnt/sdcard/... for local tests, but we push the files using the device's testroot (from the SUTAgent).  This isn't good, since technically the testroot can be different (even if it is usually /mnt/sdcard).

Really we probably don't need to be specifying the URLs at all if we specify only the root pages (e.g. startup6.html).  We can construct the URLs in the test if we organize the [htmlfiles] section better and specify just a remove server address.
Attached file proposal
Attachment #8368582 - Flags: feedback?(mcote)
I tested my idea for placing the profile on internal storage. The results weren't absolutely conclusive but I believe they warrant the change. See http://people.mozilla.org/~bclary/log/2014/02/02/autophone/index.html for details.

I can't place the profile in the fennec app directory due to limitations placed on the SUTAgent's access to files in the app directory but /data/local/tmp/profile should be adequate.
Assignee: nobody → bclary
Status: NEW → ASSIGNED
Comment on attachment 8368582 [details]
proposal

I don't really have anything to add; this sounds good both for easier customization and to potentially eliminate some noise.
Attachment #8368582 - Flags: feedback?(mcote) → feedback+
Attached patch bug-852264.patchSplinter Review
I piggy backed a change to comment out setting the device manager's reboot settling time. I left it in to make it easier to change back if we need to. Perhaps it should have been a configuration option. I also changed a couple of logger warning to logger exception calls to get more information about the exceptions in the log.
Attachment #8370802 - Flags: review?(mcote)
Oops, I left that cruft in the INSTALL about htmlfiles. Consider it removed.
Comment on attachment 8370802 [details] [diff] [review]
bug-852264.patch

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

Loving the customizability!

::: INSTALL.md
@@ +44,5 @@
> +    # the path as either a local or absolute path.
> +    #source = files/
> +
> +    # By default, Autophone will places the test files in
> +    # testroot/tests/autophone/s1test/ directory. You can customize

Might want to use angle brackets or something like that to denote that testroot is a variable.

@@ +64,5 @@
> +    # to be copied to the device. The option name is arbitrary.
> +    file1 = initialize_profile.html
> +    file2 = blank.html
> +    file3 = twitter.html
> +    file4 = twitter_files

Any reason why we don't just recursively copy the contents of the source directory, rather than specifying individual files?

::: files/initialize_profile.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<script>
> +function quit()
> +{

Teeny tiny nit: coding style dictates putting { on the line above.

@@ +9,5 @@
> +}
> +window.addEventListener("load", function () { setTimeout(quit, 1000); }, false);
> +window.onerror = quit;
> +</script>
> +<body >

Nit: superfluous space.

::: phonetest.py
@@ +99,4 @@
>                                  retryLimit=8,
>                                  logLevel=self.user_cfg['debug'])
>              # Give slow devices chance to mount all devices.
> +            #self._dm.reboot_settling_time = 0

Yeah a config option would be nice for this, unless you determine in the near future that it's not necessary at all, at which point just take it out. For now, please add a comment as to why this is commented out.

@@ +122,5 @@
> +                self.loggerdeco.exception('Attempt %d creating base device path' % attempt)
> +                sleep(self.wait_after_error)
> +
> +        if not success:
> +            raise e

Starting to wonder if this pattern can be refactored somehow, since it's been repeated a lot... just something to think about.

@@ +128,5 @@
>          return self._base_device_path
>  
>      @property
>      def profile_path(self):
> +        return self._profile_path

profile_path can probably just be a regular variable now (instead of _profile_path).

::: tests/s1s2test.py
@@ +95,5 @@
> +            try:
> +                for opt in ('source', 'dest', 'profile', 'remote'):
> +                    try:
> +                        self._paths[opt] = cfg.get('paths', opt)
> +                        if self._paths[opt][-1] != '/':

Just FYI there's a string method called .endswith() that can be very useful since you don't have to specify the length of the string you're matching against.  Doesn't really matter here because you're only checking one character, but just wanted to let you know.
Attachment #8370802 - Flags: review?(mcote) → review+
(In reply to Mark Côté ( :mcote ) from comment #6)
> Comment on attachment 8370802 [details] [diff] [review]
> bug-852264.patch
> 
> Review of attachment 8370802 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> Any reason why we don't just recursively copy the contents of the source
> directory, rather than specifying individual files?

Well, keeping it similar to the existing approach was part of it. I considered just copying the source directory contents, but kind of convinced myself that the possibility that other stuff might be in the source directory convinced me not to do it. I think it would be better though if if it's ok with you.


> ::: phonetest.py
> @@ +99,4 @@
> >                                  retryLimit=8,
> >                                  logLevel=self.user_cfg['debug'])
> >              # Give slow devices chance to mount all devices.
> > +            #self._dm.reboot_settling_time = 0
> 
> Yeah a config option would be nice for this, unless you determine in the
> near future that it's not necessary at all, at which point just take it out.
> For now, please add a comment as to why this is commented out.


I was looking at some other things as well that might be good to make configurable. I'll go ahead and comment this for now but will collect a set of settings we might want to control via the ini and file a bug for all of them at once.

> 
> @@ +122,5 @@
> > +                self.loggerdeco.exception('Attempt %d creating base device path' % attempt)
> > +                sleep(self.wait_after_error)
> > +
> > +        if not success:
> > +            raise e
> 
> Starting to wonder if this pattern can be refactored somehow, since it's
> been repeated a lot... just something to think about.

Sure. Good point.

> 
> @@ +128,5 @@
> >          return self._base_device_path
> >  
> >      @property
> >      def profile_path(self):
> > +        return self._profile_path
> 
> profile_path can probably just be a regular variable now (instead of
> _profile_path).
> 

Ok.

> ::: tests/s1s2test.py
> @@ +95,5 @@
> > +            try:
> > +                for opt in ('source', 'dest', 'profile', 'remote'):
> > +                    try:
> > +                        self._paths[opt] = cfg.get('paths', opt)
> > +                        if self._paths[opt][-1] != '/':
> 
> Just FYI there's a string method called .endswith() that can be very useful
> since you don't have to specify the length of the string you're matching
> against.  Doesn't really matter here because you're only checking one
> character, but just wanted to let you know.

Great.
(In reply to Bob Clary [:bc:] from comment #7)
> (In reply to Mark Côté ( :mcote ) from comment #6)
> > Comment on attachment 8370802 [details] [diff] [review]
> > bug-852264.patch
> > 
> > Review of attachment 8370802 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > 
> > Any reason why we don't just recursively copy the contents of the source
> > directory, rather than specifying individual files?
> 
> Well, keeping it similar to the existing approach was part of it. I
> considered just copying the source directory contents, but kind of convinced
> myself that the possibility that other stuff might be in the source
> directory convinced me not to do it. I think it would be better though if if
> it's ok with you.

Yeah, I think it's a good idea.  Chances of extra files getting in there that would negatively affect the tests is negligible IMHO.
Address review comments + a couple of issues with the previous patch. Missed setting the profile path! and missed a file:// on the initialize url. Carrying over the r+
Attachment #8371236 - Flags: review+
https://github.com/mozilla/autophone/commit/059aef2a58f815663c5703927625a9c53285038a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.