Closed Bug 1251643 Opened 4 years ago Closed 4 years ago

Investigate validating "core" pings against schemas in tests

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
2

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox47 --- affected

People

(Reporter: gfritzsche, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

We are adding a "core" ping schema in bug 1249925.
We should see whether we can add validation against this in existing or new tests.

This helps us testing the client implementation and keeping the schema up to date.
Michael, do you know if we have existing tests for the "core" pings that we could add ping validation to?
Flags: needinfo?(michael.l.comella)
Priority: P2 → P3
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Michael, do you know if we have existing tests for the "core" pings that we
> could add ping validation to?

There are no existing tests for the "core" pings.

fwiw, my current implementation thoughts for bug 1243585 are to rework the TelemetryPingGenerator into the "Builder" pattern, which would throw if the core ping is constructed without necessary fields. We could write a test against this builder object to provide validation for the schema on the client side. As such, it may be pertinent to wait for bug 1243585 to land before we invest in writing a schema validation test on the client.
Flags: needinfo?(michael.l.comella)
Thanks, that sounds good.
Depends on: 1243585
Actually, the builder landed in bug 1247489 and does validation against the required fields [1][2]. We then have tests to make sure the mandatory fields functionality is correct. [3] Georg, do you see other opportunities for testing? This seems adequate to me.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/pings/TelemetryCorePingBuilder.java#74
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/pings/TelemetryPingBuilder.java#53
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/telemetry/pings/TestTelemetryPingBuilder.java#19
Depends on: 1247489
No longer depends on: 1243585
Flags: needinfo?(gfritzsche)
I think the schema integration would be great to ensure that the client conforms to the schema and that we update the schema regularly (we currently don't since i pushed the first schema).

Would it be hard to integrate this?
If not this would probably be really useful, especially longer-term once we are integrating validation better in the data pipeline.
Flags: needinfo?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I think the schema integration would be great to ensure that the client
> conforms to the schema and that we update the schema regularly (we currently
> don't since i pushed the first schema).
> 
> Would it be hard to integrate this?

Actually, I don't understand what we're looking for here – from "ensure... that we update the schema regularly", do you mean we should check the json output and make sure it contains all of the required fields, possibly optional fields, and nothing else? That way when we add a field and run the test on the client, it will contain the new field and break the test if the schema version is not updated?
Flags: needinfo?(gfritzsche)
Assignee: nobody → michael.l.comella
The schema versioning situation is a bit undefined on the pipeline side right now.
I'll see that i get the current schema updated, but this bug doesn't seem too useful right now.

Let's close this for now and revisit later when its needed.
Flags: needinfo?(gfritzsche)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.