Closed Bug 580418 Opened 9 years ago Closed 9 years ago

Fix remote reftests to start their own webserver

Categories

(Testing :: Reftest, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

Details

Attachments

(1 file, 1 obsolete file)

For running reftests on remote devices, we need to serve them over http.  However, there are two types of reftests (for our purposes here):
1. Generic reftest that just wants to be loaded and run
2. Reftest that wants to be served over http with control over headers and other attributes of httpd.js

There are several thousand of the first type of reftest.  There are 147 of the second type at the time of this filing.

We decided to solve this in two phases, to cater to the first type of reftest we will ruse the mochitest webserver and serve them.  That's what the attached patch does.

For type 2, we'll refactor this approach so that we can use an sjs  script to do both our generic loading as well as offer the second type of reftests the control they need to run properly.  We should be able to do this without changing the tests, but that remains to be seen.
Summary: Fix remote reftests to start their own webserver for most tests → Fix remote reftests to start their own webserver
Assignee: nobody → ctalbert
Status: NEW → ASSIGNED
Attachment #458817 - Flags: review?(jmaher)
Comment on attachment 458817 [details] [diff] [review]
Patch that allows the remote reftest system to reuse the mochitest web server


>         self.add_option("--remote-logfile", action="store",
>                     type = "string", dest = "remoteLogFile",
>                     help = "Name of log file on the device relative to device root.  PLEASE USE ONLY A FILENAME.")
>-        defaults["remoteLogFile"] = "reftest.log"
>+        defaults["remoteLogFile"] = None
>+        defaults["localLogName"] = None
> 

do we have a way for the user to do a --local-logname?

> 
>     def cleanup(self, profileDir):
>+        # Pull results back from device
>+        if (self.remoteLogFile):
>+            self._devicemanager.getFile(self.remoteLogFile, self.localLogName)
>         self._devicemanager.removeDir(self.remoteProfile)
>         self._devicemanager.removeDir(self.remoteTestRoot)
>         RefTest.cleanup(self, profileDir)
> 

this is assuming there is a self.localLogName defined.  I know that it is defined and initialized above, but I would still like to see us check for the values we need here. 


Overall these are little nits.  I think for a first step this is a good method to get us up and going.  The only side effect is we now copy server.js into the reftest harness directory and we will have to make sure we clean that up in the followup bug.
Attachment #458817 - Flags: review?(jmaher) → review+
one other thing I found while doing further testing.  We require --remote-webserver on the cli, but it is not clear since we default to 'None' and check that it is not equal to '127.0.0.1'.  Can we autodetect this like we do for mochitest?
(In reply to comment #2)
> Comment on attachment 458817 [details] [diff] [review]
> Patch that allows the remote reftest system to reuse the mochitest web server
> 
> 
> >         self.add_option("--remote-logfile", action="store",
> >                     type = "string", dest = "remoteLogFile",
> >                     help = "Name of log file on the device relative to device root.  PLEASE USE ONLY A FILENAME.")
> >-        defaults["remoteLogFile"] = "reftest.log"
> >+        defaults["remoteLogFile"] = None
> >+        defaults["localLogName"] = None
> > 
> 
> do we have a way for the user to do a --local-logname?
No, and there doesn't need to be.  That's a placeholder for a quirk in the behavior due to the inheritance from runreftest.py.
> 
> > 
> >     def cleanup(self, profileDir):
> >+        # Pull results back from device
> >+        if (self.remoteLogFile):
> >+            self._devicemanager.getFile(self.remoteLogFile, self.localLogName)
> >         self._devicemanager.removeDir(self.remoteProfile)
> >         self._devicemanager.removeDir(self.remoteTestRoot)
> >         RefTest.cleanup(self, profileDir)
> > 
> 
> this is assuming there is a self.localLogName defined.  I know that it is
> defined and initialized above, but I would still like to see us check for the
> values we need here. 
>
This will be set.  But I have to go through a couple of cartwheels to get us there so that I don't step on the user's toes.  Here's the process.

--log-file can be set, defaults to None (in runreftest.py)
--remote-log-file can be set, defaults to None (in remotereftest.py)
--local-log-name cannot be set, defaults to None. (in remotereftest.py)

The code in runreftest requires that --log-file be set if we are running with a logfile.  So, I do this...

# Ensure we have a default log when running remote, regardless of what happens
if remoteLogFile == None,
  remoteLogFile = 'reftest.log' # this will be the name of the log file on the device

# Because the user is going to look for the file they specify in the --remote-log-param, set that to the localLogFile as well.
localLogFile = remoteLogFile # local log file now defaults to 'reftest.log'
remoteLogFile = testroot + / + remoteLogFile # Now that's the real remote log file

# The user could have still set --log-file, and if they did we should honor that setting locally:
if (logFile):
  localLogFile = logFile

# Ensure that logFile is set properly so that when runreftest creates the profile the logfile setting is set properly for the device:
logFile = remoteLogFile

This is all pretty convoluted, but essentially, we need a different way to specify the remoteLogFile location from the localLogFile location while still allowing the user to use the standard --log-file param. This achieves that.  The real code for this is in remotereftest.py::verifyRemoteOptions

I will make the change to auto-detect the local webserver address.
Addresses Joel's comment to have the reftest system autodetect the local IP address.  Also generalizes the code needed to grab that into a utility on the remoteAutomation class
Attachment #458817 - Attachment is obsolete: true
Attachment #459696 - Flags: review?(jmaher)
Comment on attachment 459696 [details] [diff] [review]
Adds ability to detect webserver address

hey, this looks like what I reviewed earlier with a few minor things cleaned up.
Attachment #459696 - Flags: review?(jmaher) → review+
http://hg.mozilla.org/mozilla-central/rev/2e63344e8230 -->FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.