Open
Bug 988497
Opened 7 years ago
Updated 3 years ago
Don't allow by default hardcoded ports in httpd.js
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
ASSIGNED
People
(Reporter: mihneadb, Assigned: mihneadb)
Details
Attachments
(1 file, 1 obsolete file)
1.15 KB,
patch
|
ahal
:
feedback+
|
Details | Diff | Splinter Review |
As per the discussion in bug 936181, httpd.js should not allow using hardcoded ports by default. The behavior can be changed, explicitly.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8397300 -
Flags: review?(ahalberstadt)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mihneadb
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=c042afda8000 :ahal, can you help with running this on Cedar? Thanks!
Comment 3•7 years ago
|
||
Comment on attachment 8397300 [details] [diff] [review] Don't allow by default hardcoded ports in httpd.js Review of attachment 8397300 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good to me! Yep, I'll get it landed on Cedar. ::: netwerk/test/httpserver/httpd.js @@ +518,5 @@ > + if ((typeof _ALLOW_HARDCODED_PORTS === 'undefined' || > + !_ALLOW_HARDCODED_PORTS) && > + port != -1) > + { > + dump("Cannot use hardcoded httpd.js ports in XPCShell tests.\n"); nit: theoretically this isn't necessarily XPCShell.
Attachment #8397300 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8397300 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8397330 [details] [diff] [review] Don't allow by default hardcoded ports in httpd.js Whoops! Keeping r+.
Attachment #8397330 -
Flags: review+
Comment 6•7 years ago
|
||
Oh, your try run was pushed from an old m-c rev.. I pulled m-c and did a new push: https://tbpl.mozilla.org/?tree=Try&rev=fe502197be5e Also realized that I probably shouldn't be reviewing httpd.js changes. I'll try to hunt down someone who can.. probably ted.
Updated•7 years ago
|
Attachment #8397330 -
Flags: review+ → feedback+
Comment 7•7 years ago
|
||
Comment on attachment 8397330 [details] [diff] [review] Don't allow by default hardcoded ports in httpd.js According to whimboo, Waldo can review this.. Waldo, feel free to pass the buck if that's not the case.
Attachment #8397330 -
Flags: review?(jwalden+bmo)
Comment 8•7 years ago
|
||
Comment on attachment 8397330 [details] [diff] [review] Don't allow by default hardcoded ports in httpd.js Oops, I think I should have looked at that try run before flagging :)
Attachment #8397330 -
Flags: review?(jwalden+bmo)
Comment 9•7 years ago
|
||
I guess we'll need to pass in ALLOW_HARDCODED_TESTS in mochitest. Not sure why desktop xpcshell is failing, did new tests get added that use hardcoded ports?
Assignee | ||
Comment 10•7 years ago
|
||
There were a few left to begin with. ("run-sequentially")
Comment 11•7 years ago
|
||
Oh right. Were you still planning to do the patch you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=936181#c12 ? That would be a prerequisite for this.
Assignee | ||
Comment 12•7 years ago
|
||
I thought this was the patch for that. :) I just created a separate bug since that is an intermittent failure of a particular test. We could make a patch that sets the allow_hardcoded to true for the tests that fail, but maybe some of the existing tests with issues can be improved.
Comment 13•7 years ago
|
||
Ah, ok.. I thought you were asking review because you wanted to land it. In that case we can push to Cedar, but as a prereq I'd like to not break every mochitest :)
Assignee | ||
Comment 14•7 years ago
|
||
Absolutely. That's what you suggested in the first place. :) I guess we could set that var true in the mochitest harness and take a look at the xpcshell tests first? But it would probably have to be people more knowledgeable about the tests (owners), since I remember fixing all those that I could fix.
Updated•3 years ago
|
Component: httpd.js → General
You need to log in
before you can comment on or make changes to this bug.
Description
•