Closed Bug 1413600 Opened 2 years ago Closed 2 years ago

nsAutoConfig is not tested

Categories

(Core :: AutoConfig (Mission Control Desktop), enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

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)
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)
No longer blocks: code-coverage
Summary: nsAutoConfig is not tested or is not picked up by coverage tools → nsAutoConfig is not tested
Flags: needinfo?(VYV03354)
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.
See Also: → 1435862
Status: NEW → ASSIGNED
First I'm adding a test for ReadConfig. Then I will duplicate the test for remote autoconfig.
Blocks: 1455601
Writing tests for remote autoconfig is turning out to be very painful. Still working on it.
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+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/ce20f90c85a5
Add tests for basic AutoConfig function. r=kmag
https://hg.mozilla.org/mozilla-central/rev/ce20f90c85a5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I'm not going to add testing for the remote piece at this time. I might investigate later.
(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.
Blocks: 1457215
You need to log in before you can comment on or make changes to this bug.