Remote Reftests should start their own httpd.js webserver

RESOLVED FIXED

Status

Testing
Reftest
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cmtalbert, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Created attachment 452726 [details] [diff] [review]
Remote Reftest Webserver WIP

When you run RemoteReftests they to start their own version of httpd.js automatically, the same way mochitests (both local and remote) do.

I think the real answer here is formalizing the way that we start httpd.js to make it easier to do that from any test harness.  But first, just to see how tightly coupled it is to mochitest, I'll pull it into remote reftest and then we can figure out how to go forward here.

So, pulling it into remote reftest wholesale turned out to actually be quite easy.  Here's what I did:
* Changed remotereftest as shown in the patch
* Copied httpd.js, server.js, and server-locations.txt into the reftest/ directory of a packaged test set.
* Ran remote reftest as normal.

One side effect here is because I pulled server.js over it tries to make a mochitest-style UI for the tests/ directory which is silly and slow.  But, the reftests work just fine when pointed to a manifest on the webserver.

I think what we want here is:
an HttpServer class:
  * StartServer(options, testServerData)
     ** It would instantiate its own automation from the directory's automation.py
     ** the test server data would be a dict of any specific test details needed. MochitestServer class or ReftestServer class have a couple of these details that might be factored in, but largely they both share 90% of the same code.  I think that will be the approach I'll try taking here.

I'll attach the current WIP just to get folks thinking...
Attachment #452726 - Flags: feedback?(jmaher)
very cool.  Would it make sense to do this for all reftests in general?  

In the patch, I see that you are starting the webserver with a localAutomation.  First glance I would think that we would be mixing some of the xre-path, utility-path, and profile directories between local and remote.

As we go down this path, this code should move into remoteAutomation.py since it is practically duplicated from mochitest harness.

As a note, we still need to resolve the http(...) syntax support in reftest.list files.  This is marked as a todo in reftest.js.  Just wanted to know that this doesn't resolve all reftest todo items!
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> very cool.  Would it make sense to do this for all reftests in general?  
> 
Possibly.  But I'd want to refactor everything first and be sure that we could handle the explicit serving paradigm that takes place in reftest.js before we turned this on across the board.

> In the patch, I see that you are starting the webserver with a localAutomation.
>  First glance I would think that we would be mixing some of the xre-path,
> utility-path, and profile directories between local and remote.
Yeah, that's exactly what mochitest does too.  I think it's done because Automation can only launch and monitor one process at a time.  You can't use Automation::runApp to launch httpd.js and then use the same instance of Automation::runApp to launch fennec or firefox.  I agree, sorta hacky.  

I think rather than refactoring this to be inside remoteAutomation we should refactor so that the code that encapsulates httpd.js should be its own webserver class and it should use automation.py to take advantage of the runApp etc functionality that Automation.py provides.

> 
> As a note, we still need to resolve the http(...) syntax support in
> reftest.list files.  This is marked as a todo in reftest.js.  Just wanted to
> know that this doesn't resolve all reftest todo items!
Yep, I know.  I'm wondering if there is a way to accomplish that by using (or perhaps generalizing) a server.js for reftest.  Ideally, we could take the server.js for mochitest and generalize it so that mochitest could configure the server.js the way it needs to and so that reftest could do the same.  Then we'd have a truly pluggable component here (except for that pesky xpcshell runtime dependency).
(Reporter)

Comment 3

8 years ago
Comment on attachment 452726 [details] [diff] [review]
Remote Reftest Webserver WIP

Since we chatted on the phone and decided to land this as is, and then investigate further refactoring from there, I'm asking for formal review instead of feedback.
Attachment #452726 - Flags: feedback?(jmaher) → review?(jmaher)
Comment on attachment 452726 [details] [diff] [review]
Remote Reftest Webserver WIP

>diff --git a/layout/tools/reftest/remotereftest.py b/layout/tools/reftest/remotereftest.py
> SCRIPT_DIRECTORY = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))
> sys.path.append(SCRIPT_DIRECTORY)
> #os.chdir(SCRIPT_DIRECTORY)         

please ensure you need this sys.path.append and os.chdir.  I would rather remove the os.chdir commented out line to avoid confusion in the future.


>+class ReftestServer:
>+  "Web server used to serve Reftests, for closer fidelity to the real web."

Please comment in here on the similarity with mochitest server and how this is intended only for remote testing ATM



Overall this looks good and a great way to utilize the subclasses we added earlier this year.
Attachment #452726 - Flags: review?(jmaher) → review+
(Reporter)

Comment 5

8 years ago
(In reply to comment #4)
> (From update of attachment 452726 [details] [diff] [review])
> >diff --git a/layout/tools/reftest/remotereftest.py b/layout/tools/reftest/remotereftest.py
> > SCRIPT_DIRECTORY = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))

> please ensure you need this sys.path.append and os.chdir.  I would rather
> remove the os.chdir commented out line to avoid confusion in the future.
We do actually need the SCRIPT_DIRECTORY because that's what we use to indicate to httpd.js where to place its wwwroot.  This way we can serve our test files from <currentdir>/tests/reftest etc.

This will need to get handled by the generalization of server.js in a follow on bug which I'll file.
> 
> 
> >+class ReftestServer:
> >+  "Web server used to serve Reftests, for closer fidelity to the real web."
> 
> Please comment in here on the similarity with mochitest server and how this is
> intended only for remote testing ATM
Done. 
> Overall this looks good and a great way to utilize the subclasses we added
> earlier this year.
Thanks!
(Reporter)

Comment 6

8 years ago
Forgot to mark as fixed - landed changeset: 68d98f30eda0
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.