901085 - Test for UserAgentOverrides.jsm (bug 782453)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

As part of the work for bug 897221, I'm writing an automated test for UserAgentOverrides.jsm.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentOverrides.jsm (v1)

This patch uses an SJS file to return the user agent string to the test. The test then compares the user agent strings before and after setting override prefs

Dão, do you think you can review this? Or do I need a networking peer?

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add general.useragent.enable_overrides pref to Fennec (v1)

This pref is needed to enable this test on Android, but otherwise has no effect on Fennec

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 785196 [details] [diff] [review]
Add test for UserAgentOverrides.jsm (v1)

>+const PREF_OVERRIDES_ENABLED = "general.useragent.enable_overrides";

Sigh. This pref was added in bug 835584 and it should only affect the preloading behavior. It's poorly named, we should rename it, and we should not read it in tests.

Thanks for starting this, it's good to finally have tests for this.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentOverrides.jsm (v2)

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 785196 [details] [diff] [review]
> Add test for UserAgentOverrides.jsm (v1)
> 
> >+const PREF_OVERRIDES_ENABLED = "general.useragent.enable_overrides";
> 
> Sigh. This pref was added in bug 835584 and it should only affect the
> preloading behavior. It's poorly named, we should rename it, and we should
> not read it in tests.
> 
> Thanks for starting this, it's good to finally have tests for this.

I didn't realize I could load UserAgentOverrides directly in the test. That's why I checked for that pref. Now I changed the test to load the module directly and not check the pref.

The test should now work on all platforms (except the https test on Android because https is not supported in Android mochitests). I have a try run here, https://tbpl.mozilla.org/?tree=Try&rev=2be5fba26eed

I think the test is pretty comprehensive. It tests for header UA, navigator.userAgent, subdomain support, UA regexp replacements, and enabled pref.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentOverrides.jsm (v3)

Final revision of the test

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 788322 [details] [diff] [review]
Add test for UserAgentOverrides.jsm (v3)

>+// mochitests on Android appear to have trouble with https
>+// but when it eventually works, we should re-enable the test
>+var skipHttps = /android/i.test(DEFAULT_UA);

Is there a bug filed on this?

>+skipHttps && SimpleTest.doesThrow(
>+    function () getUA('https://example.com'), 'Re-enable https test');

Convert "skipHttps &&" to an if-statement.

>+// test that UA is not overridden when the 'site_specific_overrides' pref is off
>+function testInactive(callback) {
>+    return function () {
>+        SpecialPowers.pushPrefEnv({
>+            set: [
>+                [PREF_OVERRIDES_ENABLED, false],
>+                [PREF_OVERRIDES_BRANCH + location.hostname, UA_WHOLE_OVERRIDE]
>+            ]
>+        }, function () {
>+            isnot(navigator.userAgent, UA_WHOLE_OVERRIDE,
>+                'Failed to disable navigator UA override');
>+            isnot(getUA(location.origin), UA_WHOLE_OVERRIDE,
>+                'Failed to disable header UA override');
>+            SpecialPowers.pushPrefEnv({
>+                clear: [
>+                    [PREF_OVERRIDES_ENABLED],
>+                    [PREF_OVERRIDES_BRANCH + location.hostname]
>+                ]
>+            }, callback);
>+        });
>+    };
>+}
>+
>+function testOverrides(callback) {
>+    return function () {
>+        SpecialPowers.pushPrefEnv({
>+            set: [[PREF_OVERRIDES_ENABLED, true]]
>+        }, function nextTest() {
>+            testUA(tests.shift(), function () tests.length ? nextTest() : callback());
>+        });
>+    };
>+}

Let testOverrides and testInactive run their code directly instead of returning a function and change the following:

>+testOverrides(
>+    testInactive(
>+        SimpleTest.finish
>+    )
>+)();

to this:

testOverrides(function () {
  testInactive(SimpleTest.finish);
});

>+    let params = {};
>+    request.queryString.split('&').forEach(function (val) {
>+        let sep = (val.indexOf('=') + 1) || undefined;
>+        params[val.slice(0, sep - 1)] = decodeURIComponent(val.slice(sep));
>+    });
>+
>+    switch (params.response) {
>+    case 'ua':
>+        response.write(request.getHeader('User-Agent'));
>+        break;
>+    case 'echo':
>+        response.write(params.value);
>+        break;
>+    }

The echo case appears to be unused. Can you drop all of the code dealing "params"?

Please use 2-space-indentation as per <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style>.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for UserAgentOverrides.jsm (v3.1)

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 788322 [details] [diff] [review]
> Add test for UserAgentOverrides.jsm (v3)
> 
> >+// mochitests on Android appear to have trouble with https
> >+// but when it eventually works, we should re-enable the test
> >+var skipHttps = /android/i.test(DEFAULT_UA);
> 
> Is there a bug filed on this?

Going to file one.

> >+skipHttps && SimpleTest.doesThrow(
> >+    function () getUA('https://example.com'), 'Re-enable https test');
> 
> Convert "skipHttps &&" to an if-statement.

Done

> Let testOverrides and testInactive run their code directly instead of
> returning a function and change the following:
> 
> >+testOverrides(
> >+    testInactive(
> >+        SimpleTest.finish
> >+    )
> >+)();
> 
> to this:
> 
> testOverrides(function () {
>   testInactive(SimpleTest.finish);
> });

Done

> >+    case 'echo':
> >+        response.write(params.value);
> >+        break;
> >+    }
> 
> The echo case appears to be unused. Can you drop all of the code dealing
> "params"?

"echo" was going to be used in the test for bug 897221, but now I'm going to put it into a different .sjs file for that test.

> Please use 2-space-indentation as per
> <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style>.

Done.

Carried over r+.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce671bd8e7f4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce671bd8e7f4