1249491 - Add integration tests for telemetry client ID

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I added tests for Java's client ID in bug 1244295 and Gecko in bug 1249156. However, Georg suggested some integration tests:

(In reply to Georg Fritzsche [:gfritzsche] from bug 1244295 comment #26)
> Comment on attachment 8720541 [details]
> MozReview Request: Bug 1244295 - Add regression warning to ClientID.jsm
> getClientID method. r=gfritzsche
> 
> https://reviewboard.mozilla.org/r/35351/#review32093
> 
> Are there tests (now or coming up) that cover:
> 
> * scenario 1 (creating a client id):
>     * Fennec starts on fresh profile
>     * Java code creates datareporting/state.json with a known client id
>     * Gecko starts
>     * Gecko generates and sends saved-session
>     * We assert that the client id in the ping is the same
> * scenario 2 (picking up existing client ids):
>     * Let the profile already contain
> {datareporting,healthreport}/state.json in the current format
>     * The Java code picks up the client id
>     * Trigger & receive a core ping, assert the client id
> 
> That doesn't cover everything, but should get make the dependencies
> relatively robust already.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self so I don't neglect this.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'm building on the code from bug 1253381.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/38067/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38067/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

In the test I started to write, I more or less:

String javaClientId1 = GeckoProfile.getClientID();
String jsClientId1 = (in js-land) ClientID.getClientID();
String javaClientId2 = GeckoProfile.getClientID();
String jsClientId2 = (in js-land) ClientID.getClientID();

I expect the algorithm in both to create the file if it does not exist and return the new client ID, else return the client ID from the file, i.e. javaClientId1 creates the file and the others return the same value. However, what I found is:

javaClientId1 == javaClientId2
jsClientId1 == jsClientId2
javaClientId1 != jsClientId1

So something's going wrong. :d (/me learns his lesson about writing more strict tests)

Georg, if I call out to ClientID.getClientID directly (as opposed to going through the telemetry uploader), it should just read the file Java created (assuming it created it properly and the cache hasn't been filled yet), right?

Some thoughts:
  * We could be writing/reading two files
  * JS caches the client ID value. It's possible JS is getting that value before the test starts running, at which point we delete the client ID files in an attempt to "start over" but JS is already using the cached value

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, profiles & testing is wonky in Java land – I wonder if we're reading from two different profiles.

To figure this out, I need to either log or return the file path from ClientID.jsm (if there's another API to find the profile dir, I don't know it :\) but that requires rebuilding toolkit and I'm at the day's end here. Will continue investigating tomorrow.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also, I found ClientID._reset so I started using that at the start of the test.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like the inconsistent profile dirs is the issue.

In JS (terribly escaped), via OS.Constants.Path.profileDir:
  \\\/storage\\\/emulated\\\/legacy\\\/tests\\\/profile\\

In Java:
  /data/data/org.mozilla.fennec_mcomella/files/mozilla/62swju54.testProfile/

Not sure how to reconcile off the top of my head.

Comment 8

[Georg Fritzsche [:gfritzsche]]

A side question:
Could writing the clientId to disk from Java potentially race with ClientId.jsm reading it?

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Could writing the clientId to disk from Java potentially race with
> ClientId.jsm reading it?

Yes. I raised this concern in bug 1244295 comment 3 and I felt Richard's response was reasonable:

> 1. The odds of this happening are very slim.
> 2. Any discrepancy will be resolved after a restart.
> 3. Never generate-then-store. generate-write-then-read.
> 4. Optionally, check to see if Gecko is running. If it is, ask Gecko for the ID.

To elaborate on the probability, Java will generally create the client ID as soon as the application starts: we send an Intent to start a background Service to generate and upload the ping when the application starts. Android is not guaranteed to handle this request immediately but in my testing experiences, it does.

Furthermore, my current thoughts for implementation of bug 1243585 will not create the ping in a background service so we might have even more guarantees for generating the ping.

In contrast, we at least need to wait for Gecko to start before ClientID.jsm can create the client ID (the telemetry upload generally occurs before Gecko even starts). Then, in my experiences, Gecko refrains from creating the client ID for a while (I think I waited ten minutes or so before I gave up). It's possible the implementation for this could change, however.

In the current state, the chances of these two methods running together is very low.

Notably, I chose not to implement option 4 to keep things simple. Georg, after my description above, is this something you might want?

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Michael Comella (:mcomella) from comment #9)
> In contrast, we at least need to wait for Gecko to start before ClientID.jsm
> can create the client ID (the telemetry upload generally occurs before Gecko
> even starts). Then, in my experiences, Gecko refrains from creating the
> client ID for a while (I think I waited ten minutes or so before I gave up).
> It's possible the implementation for this could change, however.

The client id in Gecko is loaded lazily on the first request [0], so it depends on whether any other code might call it early.

> In the current state, the chances of these two methods running together is
> very low.
> 
> Notably, I chose not to implement option 4 to keep things simple. Georg,
> after my description above, is this something you might want?

I can't judge how likely/unlikely this race is, but if we have a chance to avoid diverging client ids, that would be great.
Is it maybe easier to have ClientID.jsm request the client id from the Java parts?

Sorry for side-tracking the bug, maybe it makes more sense to take it to a new bug.

0: https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/toolkit/modules/ClientID.jsm#160

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> I can't judge how likely/unlikely this race is, but if we have a chance to
> avoid diverging client ids, that would be great.
> Is it maybe easier to have ClientID.jsm request the client id from the Java
> parts?

Filed bug 1255657.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8726510 [details]
MozReview Request: Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38067/diff/1-2/

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8726510 [details]
MozReview Request: Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38067/diff/2-3/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1249491 - Move getTestProfile to BaseRobocopTest. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39899/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39899/

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39901/

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8730469 [details]
MozReview Request: Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

Georg, if you'd like to look over what I tested (optional), the java file should provide a good overview.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 18

[Georg Fritzsche [:gfritzsche]]

https://reviewboard.mozilla.org/r/39901/#review36581

This looks sensible to me except for the missing `"healthreport/state.json"` coverage.
I'm not sure how many of those we still have around, but unless we decide to drop that migration we should not break it.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testUnifiedTelemetryClientId.java:17
(Diff revision 1)
> +import java.io.IOException;
> +
> +public class testUnifiedTelemetryClientId extends JavascriptBridgeTest {
> +    private static final String TEST_JS = "testUnifiedTelemetryClientId.js";
> +
> +    private static final String CLIENT_ID_PATH = "datareporting/state.json";

What about the `"healthreport/state.json"` path?
Should that have coverage here too?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testUnifiedTelemetryClientId.java:110
(Diff revision 1)
> +        fAssertEquals("Same client ID retrieved from Java", clientIdFromJS, clientIdFromJavaAgain);
> +    }
> +
> +    private String getClientIdFromJava() throws IOException {
> +        // This assumes implementation details: it assumes the client ID
> +        // file is created when Java attempts to retrieve it if it DNE.

DNE? Worth writing that out, took me a moment.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8726510 [details]
MozReview Request: Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

https://reviewboard.mozilla.org/r/38067/#review37345

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/JavascriptBridgeTest.java:65
(Diff revision 3)
> +     *
> +     * Ideally, we could just have Javascript call Java when the callback completes but Java won't
> +     * listen for messages unless we call into JS again (bug 1253467).
> +     *
> +     * To use this method:
> +     *   * Call this method will a name argument, henceforth known as `varName`. Note that it will be capitalized

I asumme "will a name argument" should be "with a name argument"?

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730468 [details]
MozReview Request: Bug 1249491 - Move getTestProfile to BaseRobocopTest. r=sebastian

https://reviewboard.mozilla.org/r/39899/#review37347

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730469 [details]
MozReview Request: Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

https://reviewboard.mozilla.org/r/39901/#review37357

Let me know if yield would be helpful here (see below). If not then just reflag me for review because then I clearly haven't understood what's going on. :)

::: mobile/android/tests/browser/robocop/testUnifiedTelemetryClientId.js:17
(Diff revision 1)
> +    isClientIDSet = false;
> +    ClientID.getClientID().then(function (retClientID) {
> +        // Ideally, we'd directly send the client ID back to Java but Java won't listen for
> +        // js messages after we return from the containing function (bug 1253467).
> +        clientID = retClientID;
> +        isClientIDSet = true;
> +    }, function (fail) {
> +        // Since Java doesn't listen to our messages (bug 1253467), I don't expect
> +        // this throw to work correctly but we should timeout in Java.
> +        do_throw('Could not retrieve client ID: ' + fail);
> +    });

You write that Java won't listen for messages after the method returns. But couldn't you make the promise synchronous by using yield and then execute the callback directly?

Someting like:

let clientId = yield ClientID.getClientID();
java.asyncCall('...');

With yield it should block until the value is available and then the method wouldn't return early?

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/39901/#review37357

> You write that Java won't listen for messages after the method returns. But couldn't you make the promise synchronous by using yield and then execute the callback directly?
> 
> Someting like:
> 
> let clientId = yield ClientID.getClientID();
> java.asyncCall('...');
> 
> With yield it should block until the value is available and then the method wouldn't return early?

When I initially wrote this test, I thought there was a way to synchronously resolve Promises with yield and tried my best but I couldn't get it to work. When I asked around the office, "How can I synchronously resolve Promises?", everyone said, "Don't do that." The reason being the thread needs to JS thread needs to yield in order to resolve a Promise and it only does so when the current function (and it's calling functions) have returned.

That being said, I find out how Promises were resolved synchronously in the past: [Task.js](http://taskjs.org/). Unfortunately, I also couldn't get this to work – I added a comment. Disclaimer: I tried this only briefly but I'd rather these tests work now than work elegantly later – there's still a lot of telemetry work to do.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8726510 [details]
MozReview Request: Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38067/diff/3-4/

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8730468 [details]
MozReview Request: Bug 1249491 - Move getTestProfile to BaseRobocopTest. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39899/diff/1-2/

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8730469 [details]
MozReview Request: Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39901/diff/1-2/

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: ROLL - try increasing wait timeout to get tests running in try.

Review commit: https://reviewboard.mozilla.org/r/42349/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42349/

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1249491 - Add health report client ID migration test. r=sebastian,gfritzche

Review commit: https://reviewboard.mozilla.org/r/42351/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42351/

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 31

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #22)

> That being said, I find out how Promises were resolved synchronously in the
> past: [Task.js](http://taskjs.org/). Unfortunately, I also couldn't get this
> to work – I added a comment. Disclaimer: I tried this only briefly but I'd
> rather these tests work now than work elegantly later – there's still a lot
> of telemetry work to do.

FYI, there's task support in robocop_head.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/robocop_head.js

testTrackingProtection has some examples of this in use:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testTrackingProtection.js

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #31)
> FYI, there's task support in robocop_head.js:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/
> robocop/robocop_head.js

I don't think this will work for this case – `add_task` acts as a test harness where it takes a function, wraps it in Task.spawn, and queues it to be run. However, the functions that can be called from Java via JavascriptBridge are top-level functions and don't really fit into the `add_task` test harness.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730469 [details]
MozReview Request: Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

https://reviewboard.mozilla.org/r/39901/#review39421

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8734553 [details]
MozReview Request: Bug 1249491 - Add health report client ID migration test. r=sebastian,gfritzche

https://reviewboard.mozilla.org/r/42351/#review39423

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/14c2f0dda69563fabc9a19c6aa1d076ae59592be
Bug 1249491 - Add JavascriptBridgeTest.getBlockingFromJsString and friends. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/08685d4867990cf629a57baaeeb24f66575cd296
Bug 1249491 - Move getTestProfile to BaseRobocopTest. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/1e683499ef1155866eb34b57d01b0e8c0e337fa3
Bug 1249491 - Write java/js integration test for retrieving telemetry client ID. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/416d7925fc2df25bb85e26bee3fd78605a312566
Bug 1249491 - Add health report client ID migration test. r=sebastian,gfritzche

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14c2f0dda695
https://hg.mozilla.org/mozilla-central/rev/08685d486799
https://hg.mozilla.org/mozilla-central/rev/1e683499ef11
https://hg.mozilla.org/mozilla-central/rev/416d7925fc2d