Closed
Bug 852264
Opened 12 years ago
Closed 11 years ago
[s1s2] don't hardcode URLs for local test files
Categories
(Testing Graveyard :: Autophone, defect)
Testing Graveyard
Autophone
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcote, Assigned: bc)
Details
Attachments
(3 files)
|
4.00 KB,
text/plain
|
mcote
:
feedback+
|
Details |
|
15.79 KB,
patch
|
mcote
:
review+
|
Details | Diff | Splinter Review |
|
15.92 KB,
patch
|
bc
:
review+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•11 years ago
|
||
Attachment #8368582 -
Flags: feedback?(mcote)
|
|
Assignee | |
Comment 2•11 years ago
|
||
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
|
Reporter | |
Comment 3•11 years ago
|
||
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+
|
|
Assignee | |
Comment 4•11 years ago
|
||
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)
|
|
Assignee | |
Comment 5•11 years ago
|
||
Oops, I left that cruft in the INSTALL about htmlfiles. Consider it removed.
|
|
Reporter | |
Comment 6•11 years ago
|
||
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+
|
|
Assignee | |
Comment 7•11 years ago
|
||
(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.
|
|
Reporter | |
Comment 8•11 years ago
|
||
(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.
|
|
Assignee | |
Comment 9•11 years ago
|
||
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+
|
|
Assignee | |
Comment 10•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
|
||
Updated•3 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•