Closed Bug 1413600 Opened 2 years ago Closed 2 years ago

nsAutoConfig is not tested


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

Not set



Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed


(Reporter: marco, Assigned: mkaply)


(Blocks 2 open bugs)



(1 file)

See discussion in dev-platform:

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 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
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.


::: 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[";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
Add tests for basic AutoConfig function. r=kmag
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.