983231 - Telemetry experiments: test manifest condition evaluation

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

From bug 974009, test_conditions.js has to be extended with good coverage of the manifest conditions.

Comment 1

[Georg Fritzsche [:gfritzsche]]

This tests around a simple payload:
http://dxr.mozilla.org/mozilla-central/source/services/healthreport/tests/xpcshell/test_healthreporter.js#293

We should be able to test jsfilter properly once we have that and something similar for telemetry as well.

Comment 2

[Georg Fritzsche [:gfritzsche]]

I'm not sure if you looked into this yet, but i had to fixup the tests a little - i updated the test_conditions.js on bug 974009.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Ok, this should be done on top of the patches for bug 974009 and bug 983226.
The behavior of bug 974009 should be stable now as far as this bug is concerned. The changes done there should only require a little merging with any work done here previously.


Going through the spec in bug 973997 i can think of the following cases, influencing conditions, etc.:

== Time conditions: ==

They depend on experiment activation state (never enabled, currently enabled, was enabled), which affects _startDate, _endDate, _enabled.

startTime: <now, ==now, >now, nonsense
maxStartTime:  <now, ==now, >now, <startTime, ==startTime, >startTime, nonsense
endTime: <startTime, ==startTime, >startTime, nonsense
maxActiveSeconds: startTime+maxActiveSeconds<now, startTime+maxActiveSeconds==now, startTime+maxActiveSeconds>now, nonsense

== Environment ==

appName, version, buildIDs, os, channel, locale:
  in list, not in list, nonsense. Faked via createAppinfo() or policy. Moving more to the policy would be cleaner.

minVersion, maxVersion, minBuildID, maxBuildID:
 <, ==, >, nonsense

== Experiment control ==

disabled, frozen: 
  interaction matrix: frozen, disabled, experiment active or not, otherwise applicable or not

== Other ==

sample: <fakeValue, ==fakeValue, >fakeValue, nonsense. Result should persist over multiple calls
  |patchPolicy(gPolicy, {random: () => fakeValue});|

jsfilter:
  Should have ok coverage now per bugs 974009 and 983226?
  truthy, falsy & undefined results. parsing error, exceptions, rejection reasons from inside function.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Extended coverage for manifest conditions and fixes.

Note: compared to the above comment i mostly skipped the "nonsense" test-cases as a) we haven't defined the behavior for those cases and b) in some cases validity is defined by other components (e.g. build ids, or versions).

try: https://tbpl.mozilla.org/?tree=Try&rev=ea475d99abd2

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8397123 [details] [diff] [review]
Extended coverage for manifest conditions and fixes.

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

::: browser/experiments/test/xpcshell/head.js
@@ +168,5 @@
>    let version = options.version || "1.0";
>    let platformVersion = options.platformVersion || "1.0";
>    let date = options.date || new Date();
>  
> +  let buildID = options.buildID || "2014060601";

can you define this string as a const at the top of the file and reuse it here and in test_conditions.js?  Actually you don't even need it in test_conditions.js due to this being the default value of none is given. But it still good to have it as a const

Comment 6

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/b020204faeb9

Comment 7

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

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