Serve reftests from a web server so as not to depend on external resources



12 years ago
10 years ago


(Reporter: Waldo, Assigned: Waldo)


Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



12 years ago
Two reftests require certain HTTP status codes and thus depend on external resources.  If those external resources are unreachable, the tests fail and will intermittently turn testing tinderboxen orange:

REFTEST UNEXPECTED FAIL (LOADING): file:///build/slave/trunk_linux/mozilla/layout/reftests/object/404-data.html
REFTEST UNEXPECTED FAIL (LOADING): file:///build/slave/trunk_linux/mozilla/layout/reftests/object/404-data-with-type.html

Other reftests (not written or not yet added to the test suite) require certain HTTP headers which can't reasonably be inferred from the file itself (usually a non-default Content-Type).,32-36#14

The only way to solve this is to serve the files from a non-remote web server and change the status/headers as necessary when each file is served.  We have a simple web server at <netwerk/test/httpserver/httpd.js> which can serve the files, and it should be fairly easy to use it to serve reftests and supporting files.  The server doesn't yet support header/status munging for static files, but I have a patch in bug 367214 to do so.

(I should also note that we don't have to and shouldn't run every test over HTTP, since the server can never be more performant than file: URLs; we can annotate the specific tests in reftest.list which require a server and use it only for those tests.)
This seems like a pretty big/intrusive change to fix a small number of tests.  Are you planning to do it in a way that changes the way one has to use the reftest harness (how much harder will it make running the whole set, or a subset?), or were you planning to make the reftest harness able to start the internal http server?

Comment 2

12 years ago
The latter; the server has the trappings to be used as an XPCOM component or as an inline script, so we can either use the subscript loader to use it or can include it directly in the reftest XPI components/ directory to make it usable.  The server itself (and other components, but you won't need them here) has a small, well-defined interface, and I suspect the patch will be at most 10K or so.

Comment 3

12 years ago
Created attachment 260752 [details] [diff] [review]
Mostly but not totally done

This patch sorta works, if hackishly, and it should give an idea of how little reftest must be changed to actually run tests from a web server.  You'll need to apply the patch in bug 367214 for this to work.  My test machine is being quirky with test execution, but this seems to at least mostly work.

With this and the other patch, to modify headers/HTTP statuses for a file FOO, create a file _headers_FOO as a sibling.  Optionally start it with a line "HTTP ###" where ### is the desired HTTP status code (if this line's not present, defaults to 200), and include standard HTTP header lines after that to set new headers or replace existing ones.

This patch needs cleanup before review, but feel free to read it if you want.

Comment 4

11 years ago
Created attachment 267629 [details] [diff] [review]
Patch, with a couple tests run over HTTP

Okay, this should be a bit cleaner, and I think it's ready to be reviewed.  I've included some modifications to existing tests to demonstrate the HTTP functionality in use, and I've tested that the new test I added that uses HTTP correctly fails without HTTP and correctly succeeds with it.

A few other notes:

- only tests explicitly marked as run-over-HTTP are done over HTTP
- the reftest output displays the file URL for the test in its output,
  even if the test is done over HTTP, because of the URL munging used
  to map the test onto the server; if you have better suggestions for
  the displayed UI, I'd love to hear them
- feedback on the README.txt changes would be appreciated, if anything's unclear
Attachment #260752 - Attachment is obsolete: true
Attachment #267629 - Flags: review?(dbaron)
Comment on attachment 267629 [details] [diff] [review]
Patch, with a couple tests run over HTTP

r+sr=dbaron.  Sorry for the long delay here.
Attachment #267629 - Flags: superreview+
Attachment #267629 - Flags: review?(dbaron)
Attachment #267629 - Flags: review+

Comment 6

11 years ago
Created attachment 272841 [details] [diff] [review]
Small-ish tweaks to deal with checkLoadURI additions since patch creation

A checkin after I made the previous patch added some checkLoadURI calls; I added what I think are the correct calls to this updated patch, but I'd like a double-check to be safe.  To be honest I'm not even sure the calls do anything in the HTTP case, but I'll let you say not to do them if necessary.

Unfortunately, because the file versions differ, I doubt Bugzilla interdiff will be useful here.
Attachment #272841 - Flags: review?(dbaron)
Attachment #272841 - Flags: review?(dbaron) → review+

Comment 7

11 years ago
Fixed.  I don't see docs for reftests outside of the in-tree readme which aim for the level of detail needed to care about being able to do HTTP stuff, so I don't think there are any further documentation changes to make for this.
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9alpha4 → mozilla1.9beta1
Component: Layout: Misc Code → Reftest
Flags: in-testsuite+
Product: Core → Testing
Target Milestone: mozilla1.9alpha8 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.