Closed
Bug 1413600
Opened 7 years ago
Closed 6 years ago
nsAutoConfig is not tested
Categories
(Core :: AutoConfig (Mission Control Desktop), enhancement)
Core
AutoConfig (Mission Control Desktop)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: marco, Assigned: mkaply)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
See discussion in dev-platform: https://groups.google.com/d/msg/mozilla.dev.platform/fr1l2PA7kL8/NI7TDbdgCQAJ. We need to understand whether this is a problem in the coverage tools (maybe we are not executing the test in the coverage build?) or if the file actually is not tested. As far as I can tell, the test in http://searchfox.org/mozilla-central/source/extensions/pref/autoconfig/test/unit is only testing nsReadConfig and not nsAutoConfig, as nsAutoConfig is not instantiated by nsReadConfig unless the "autoadmin.global_config_url" preference is set. Am I missing something?
Flags: needinfo?(mozilla)
Flags: needinfo?(VYV03354)
Reporter | ||
Updated•6 years ago
|
Blocks: untestedcode-codecoverage
Assignee | ||
Comment 1•6 years ago
|
||
You are correct. nsAutoConfig is only used when the config file is at a URL (the file is poorly named). Note that this can be done with a file URL - it doesn't have to be an HTTP URL. nsReadConfig is really the core autoconfig functionality. Unfortunately none of this stuff is well covered with tests.
Flags: needinfo?(mozilla)
Reporter | ||
Updated•6 years ago
|
No longer blocks: code-coverage
Summary: nsAutoConfig is not tested or is not picked up by coverage tools → nsAutoConfig is not tested
Updated•6 years ago
|
Flags: needinfo?(VYV03354)
Comment 2•6 years ago
|
||
https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds
status-firefox59:
--- → ?
Assignee | ||
Comment 3•6 years ago
|
||
So I'm at a loss as to how to write a good test for this specific case. We would need to set autoadmin.global_config_url to a file URL pointing to an autoconfig that set a value. I'm sure someone that understood the tests well could probably do this.
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
First I'm adding a test for ReadConfig. Then I will duplicate the test for remote autoconfig.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Writing tests for remote autoconfig is turning out to be very painful. Still working on it.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8968954 [details] Bug 1413600 - Add tests for basic AutoConfig function. https://reviewboard.mozilla.org/r/237654/#review244872 Nice ::: extensions/pref/autoconfig/test/unit/test_autoconfig.js:12 (Diff revision 2) > + > +ChromeUtils.import("resource://gre/modules/Services.jsm"); > + > +function run_test() { > + let env = Cc["@mozilla.org/process/environment;1"]. > + getService(Ci.nsIEnvironment); Nit: "." on the same line as the property name in new code, please.
Attachment #8968954 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/ce20f90c85a5 Add tests for basic AutoConfig function. r=kmag
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce20f90c85a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 12•6 years ago
|
||
I'm not going to add testing for the remote piece at this time. I might investigate later.
Reporter | ||
Comment 13•6 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #12) > I'm not going to add testing for the remote piece at this time. I might > investigate later. Can you file another bug blocking bug 1415824? So we don't forget this is not done yet.
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•