1240868 - TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js (perma-fail on comm-central)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Magnus Melin [:mkmelin]]

http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-linux64/1453219179/comm-central_ubuntu64_vm_test-xpcshell-bm118-tests1-linux64-build1.txt.gz:

10:08:12     INFO -  toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | Starting test_collectionWithbrokenAddonData
10:08:12     INFO -  (xpcshell/head.js) | test test_collectionWithbrokenAddonData pending (3)
10:08:12     INFO -  (xpcshell/head.js) | test run_next_test 11 finished (3)
10:08:12     INFO -  Unexpected exception NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
10:08:12     INFO -  test_collectionWithbrokenAddonData@/builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1101:16
10:08:12     INFO -  _run_next_test@/builds/slave/test/build/tests/xpcshell/head.js:1541:9
10:08:12     INFO -  do_execute_soon/<.run@/builds/slave/test/build/tests/xpcshell/head.js:681:9
10:08:12     INFO -  _do_main@/builds/slave/test/build/tests/xpcshell/head.js:208:5
10:08:12     INFO -  _execute_test@/builds/slave/test/build/tests/xpcshell/head.js:541:5
10:08:12     INFO -  @-e:1:1
10:08:12     INFO -  exiting test

Leads to http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#1101

So it's because we're not shipping Social.jsm which is a browser/ module.

Comment 1

[Magnus Melin [:mkmelin]]

From bug 1238142. 

How do you want to handle this? It's a toolkit/ test that now tests a browser/ module.

Comment 2

[Shane Caraveo (:mixedpuppy)]

That seems pretty wrong and should be changed.

See toolkit/components/social/test/browser/browser_SocialProvider.js for an example of adding a provider without the browser code, or toolkit/components/social/test/xpcshell/browser_SocialService.js for an xpcshell example.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Alternative (if bug 1238142 is being uplifted), land/uplift bug 1240682 instead (though frankly the telemetry code should still be hardened).

Comment 4

[Shane Caraveo (:mixedpuppy)]

hmm, additionally that test might break given the fix in bug 1240682.  I think the test should validate bad data going into the telemetry code without relying on socialapi code.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> hmm, additionally that test might break given the fix in bug 1240682.  I
> think the test should validate bad data going into the telemetry code
> without relying on socialapi code.

Right, we should just setup a mock provider that exhibits the broken behavior.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1240868.patch

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8709931 [details] [diff] [review]
bug1240868.patch

Review of attachment 8709931 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +122,5 @@
>  }
>  
> +function MockAddonWrapper(aManifest) {
> +  this.manifest = aManifest;
> +  this.id = aManifest.id;

Why is this a normal property instead of just a getter like the others?

@@ +133,5 @@
> +  get appDisabled() {
> +    return false;
> +  },
> +
> +  get softDisabled() {

A few of the properties are not actually required:
https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/Addon#Required_properties

Lets keep it to the minimum here.

@@ +226,5 @@
> +    get name() {
> +      return aName;
> +    },
> +
> +    addAddon: function(aAddon) {

Here it is called |addon|, elsewhere |manifest|.
Lets stick with one of the terms.

@@ +239,5 @@
> +    shutdown() {
> +      return Promise.resolve();
> +    },
> +  };
> +  return mockProvider;

Nit: newline before this line.

@@ +1221,2 @@
>    let checkpointPromise = registerCheckpointPromise(1);
> +  // Register the broken provider and install the broken addon.

Newline before this comment.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1240868.patch

Thanks for the review, there we go.

Comment 9

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f11389063a

Comment 10

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=004a01ec00e0

Comment 11

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/9571a4bd71d1f813b632b3624874a3c666ab20ac
Bug 1240868 - Use a mock addon provider to feed broken data in Telemetry. r=gfritzsche

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9571a4bd71d1

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8709976 [details] [diff] [review]
bug1240868.patch

Approval Request Comment
[Feature/regressing bug #]: 1238142
[User impact if declined]: None. This fixes a test that permafails comm-central and should be uplifted along with 1238142, stacking on it.
[Describe test coverage new/current, TreeHerder]: This patch changes a test so that it doesn't use the SocialAPI provider.
[Risks and why]: Low risk. This patch changes a test so that it doesn't use the SocialAPI provider.
[String/UUID change made/needed]: None.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Taking it to improve the status of the testsuite

Comment 15

[Carsten Book [:Tomcat]]

has problems during uplift:

grafting 324721:9571a4bd71d1 "Bug 1240868 - Use a mock addon provider to feed broken data in Telemetry. r=gfritzsche"
merging toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
warning: conflicts while merging toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0cd0ae95d865

Comment 17

[Carsten Book [:Tomcat]]

sorry this needed bug 1238142  it seems :)

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Moving to toolkit so I can get this out of the uplift query.

Comment 19

[Intermittent Failures Robot]

16 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 16

Platform breakdown:
* windowsxp: 4
* osx-10-10: 4
* windows7-32: 3
* linux64: 3
* osx-10-6: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1240868&startday=2016-01-18&endday=2016-01-24&tree=all