Closed
Bug 675616
Opened 13 years ago
Closed 13 years ago
Trivial fix in layout/style/test/test_css_cross_domain.html
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla8
People
(Reporter: mayhemer, Unassigned)
Details
(Whiteboard: [inbound])
Attachments
(1 file)
(deleted),
patch
|
zwol
:
review+
|
Details | Diff | Splinter Review |
As part of the work to make httpd.js support keep-alive connections this test needs a little fix: change using of localhost:8888 to proper mochi.test:8888.
Otherwise the test cause assertions in httpd.js and fails.
Attachment #549781 -
Flags: review?(zackw)
Comment 1•13 years ago
|
||
It does what it says, but could you please explain why localhost is no longer usable?
Reporter | ||
Comment 2•13 years ago
|
||
Actually I start thinking I have to rerun the test w/o this fix. According to my notes this test had a problem but it had been fixed by another change in the tweaked httpd.js.
I will let you know and potentially close this bug as INVALID. However, I think fixing this is anyway good think to do.
Reporter | ||
Comment 3•13 years ago
|
||
Yes, this fix is not needed to allow httpd.js to support keep-alive connections. Two passes of all layout/style tests and the test is OK w/o the proposed fix.
However I can see on the server following line in the log:
*** unrecognized hostname (localhost:8888) in Host header, 400 time
for GET /tests/layout/style/test/ccd.sjs?JD2ls (and similar) request. It means, the server returns 400 Bad request as a response. If this is intentional, then let's close this bug as INVALID.
Reporter | ||
Comment 4•13 years ago
|
||
According to recheck on this fix, this no longer blocks bug 469228.
No longer blocks: 469228
Comment 5•13 years ago
|
||
Comment on attachment 549781 [details] [diff] [review]
v1
(In reply to comment #3)
> Yes, this fix is not needed to allow httpd.js to support keep-alive
> connections. Two passes of all layout/style tests and the test is OK w/o
> the proposed fix.
>
> However I can see on the server following line in the log:
>
> *** unrecognized hostname (localhost:8888) in Host header, 400 time
>
> for GET /tests/layout/style/test/ccd.sjs?JD2ls (and similar) request. It
> means, the server returns 400 Bad request as a response. If this is
> intentional, then let's close this bug as INVALID.
I don't believe that's intentional, and it might explain the mysterious intermittent failures seen in bug 536603. It sounds like a bug in httpd.js, though - why should name resolution of 'localhost' ever fail?
Independent of all that, I am going to r+ your patch simply because there's no good reason for ccd-standards.html to be inconsistent with ccd-quirks.html.
Attachment #549781 -
Flags: review?(zackw) → review+
Comment 6•13 years ago
|
||
... unless the server is deliberately throwing 400 Bad Request for *all* requests that specify Host: localhost because at some point there was a decision that tests should always use mochi.test instead. (I am unaware of any such decision, but I have not been paying close attention to test-writing policy lately.) In which case your patch fixes a bug rather than a mere inconsistency, the only thing wrong with httpd.js is it isn't clear enough about its diagnostics, and I'm a bit mystified why the test doesn't fail all the time.
Comment 7•13 years ago
|
||
It's been awhile, but I do believe the intent was to completely switch to mochi.test and not use localhost at all. The problem is that Mochitest on mobile systems runs the web server on a remote machine, not the local machine, so "localhost" is a quite inaccurate moniker in that situation. (localhost also requires extra-special work to proxy, since it usually should resolve to 127.0.0.1.) The 400 error is the proper response when the Host included in the incoming request is unrecognized. httpd.js includes some special logic to make localhost always work, at least by default; it might be the case that that logic also kicks in for Mochitest and that we should do extra work to disable it.
Comment 8•13 years ago
|
||
...or we could just ask the guy who made the localhost->mochi.test switch, of course. :-)
Comment 9•13 years ago
|
||
we switches localhost->mochi.test for windowsCE because there was no localhost and we were running into problems (i.e. special handling of localhost and PAC settings). All mobile mochitests run on a remote server instead of on the device.
Comment 10•13 years ago
|
||
Hm, ok, in that case test_css_cross_domain definitely should change, and maybe httpd.js's special casing for localhost should change too (to always spit an error).
I realized why this doesn't make the test fail: the test can't tell the difference between "successfully downloaded the style sheet and then discarded it because it had the wrong MIME type" and "failed to download the style sheet." Probably not worth trying to fix that.
Reporter | ||
Comment 11•13 years ago
|
||
(In reply to comment #10)
> I realized why this doesn't make the test fail: the test can't tell the
> difference between "successfully downloaded the style sheet and then
> discarded it because it had the wrong MIME type" and "failed to download the
> style sheet." Probably not worth trying to fix that.
It is doable: register "examine-response" observer and check the result/status of the channel for you CSS.
But really, probably not worth to do this ;)
To confirm: patch is actually r+'ed and free to land?
Comment 12•13 years ago
|
||
> To confirm: patch is actually r+'ed and free to land?
Yes.
Reporter | ||
Comment 13•13 years ago
|
||
Whiteboard: [inbound]
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Assignee | ||
Updated•7 years ago
|
Component: httpd.js → General
You need to log in
before you can comment on or make changes to this bug.
Description
•