Closed Bug 1240868 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: mkmelin, Assigned: Dexter)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

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.
From bug 1238142. 

How do you want to handle this? It's a toolkit/ test that now tests a browser/ module.
Blocks: 1238142
Keywords: regression
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.
Alternative (if bug 1238142 is being uplifted), land/uplift bug 1240682 instead (though frankly the telemetry code should still be hardened).
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.
Priority: -- → P1
Whiteboard: [measurement:client]
(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.
Attached patch bug1240868.patch (obsolete) — Splinter Review
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8709931 - Flags: review?(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.
Attachment #8709931 - Flags: review?(gfritzsche)
Attached patch bug1240868.patchSplinter Review
Thanks for the review, there we go.
Attachment #8709931 - Attachment is obsolete: true
Attachment #8709976 - Flags: review?(gfritzsche)
Attachment #8709976 - Flags: review?(gfritzsche) → review+
https://hg.mozilla.org/integration/fx-team/rev/9571a4bd71d1f813b632b3624874a3c666ab20ac
Bug 1240868 - Use a mock addon provider to feed broken data in Telemetry. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/9571a4bd71d1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
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.
Attachment #8709976 - Flags: approval-mozilla-aurora?
Attachment #8709976 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Taking it to improve the status of the testsuite
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!
Flags: needinfo?(alessio.placitelli)
sorry this needed bug 1238142  it seems :)
Flags: needinfo?(alessio.placitelli)
Moving to toolkit so I can get this out of the uplift query.
Component: General → Telemetry
Product: Thunderbird → Toolkit
Target Milestone: Thunderbird 46.0 → ---
You need to log in before you can comment on or make changes to this bug.