|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
We currently have some try/catch blocks in the shutdown procedure of the TelemetrySend object to cope with tests that call shutdown() without calling setup() first or that call shutdown() twice in a row, see: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/telemetry/TelemetrySend.jsm#642 This makes the tests output rather confusing as they produce a number of warnings even when the tests succeed. The tests should be fixed so that they act consistently and we don't need to ignore errors this way.
I've looked into this but I haven't found any tests that trigger the try/catch you link to. I know it was a while ago, but do you recall which tests were exhibiting this error? It's possible that they got cleaned up independently of this bug, I imagine.
Oops, I was looking at the output from running ./mach test toolkit/components/telemetry/tests/unit/ and so I was missing the more detailed output. I added --verbose and now I can get the full output for the whole directory. Thanks!
For my records these are the tests showing these errors: test_MigratePendingPings test_PingSender test_TelemetryControllerShutdown test_TelemetryEnvironment test_TelemetryEvents test_TelemetryFlagClear test_TelemetryHistograms test_TelemetryLateWrites test_TelemetryLockCount test_TelemetryLog test_TelemetryScalars test_TelemetrySend test_TelemetrySession_activeTicks test_TelemetryStopwatch test_TelemetryStorage test_TelemetryTimestamps
Changing the function given to do_register_cleanup fixes almost all of those tests. Then I just had to make one more change, to setup before the shutdown in test_sendCheckOverride. This seems to handle all instances of the error and seems to continue to test OK. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59bd4015597f3aa0fc29ae98feb0c78e727ea64a
Comment on attachment 8935567 [details] Bug 1348890 - Call setup()/shutdown() consistently Redirecting the review to a telemetry peer. Glad to see this fixed, the logs when running these tests will be a lot cleaner. Also, this try/catch block should probably go too since it was there only to accommodate for the fact that shutdown() was being called multiple times: https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/telemetry/TelemetrySend.jsm#753 It should now be possible to just remove the observers without having to worry that the operation might throw.