Refactor FXIOS-13671 [Telemetry & Unit tests] Fix telemetry crashes in unit tests #29799

lmarceau · 2025-10-03T16:58:22Z

📜 Tickets

💡 Description

Testing some ideas to fix telemetry crashing in unit tests. We have many (hundreds) of runtime warnings when unit tests are run. Some relates to Glean, some relates to Nimbus.

Examples

Opinions

I think we shouldn't call TelemetryWrapper events when we are unit testing, unless this is specifically wanted. My reasoning is we often call telemetry events as side-effect of unit testing some code. There's no guarantee Glean is setup for testing when we call those events, so I am adding a hasTelemetryOverride for unit testing purpose.
Flaky tests are worst than no tests, so I am disabling some unit tests in this PR and want to create follow up tasks so we either use MockGleanWrapper or other mocking mechanism in place to avoid calling Glean directly.
I am adding someXCTestCase extensions to help setup and teardown telemetry
With those changes we have 1 runtime only warning left when running unit tests
Modified testEventMetricRecordingSuccess so it's using an XCTUnwrap instead of force-unwrapping which meant a bunch of change in many files

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

mobiletest-ci-bot · 2025-10-03T17:09:09Z

	Messages
📖	Project coverage: 39.03%

💪 Quality guardian

21 tests files modified. You're a champion of test coverage! 🚀

🧩 Neat Piece

This PR changes 601 lines. It's a substantial update,
but still review-friendly if there’s a clear description. Thanks for keeping things moving! 🚀

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ Per-file coverage

All changed files meet the threshold of 35.0%.

Client.app: Coverage: 37.21

File	Coverage
PingUploaderProtocol.swift	100.0%	✅
TelemetryWrapper.swift	64.38%	✅
GleanHttpUploader.swift	95.16%	✅

Generated by 🚫 Danger Swift against 8ea301c

lmarceau · 2025-10-03T17:36:37Z

firefox-ios/Client/Utils/GleanHttpUploader.swift

            uploadTask.resume()
        } else {
-            logger.log("Rejected ohttp ping since couldn't build request", level: .info, category: .telemetry)
+            logger.log("Rejected http ping since couldn't build request", level: .info, category: .telemetry)


Fixing typo

lmarceau · 2025-10-03T17:40:17Z

firefox-ios/Client/Utils/PingUploaderProtocol.swift

-        // NOTE: We're using `URLSession.uploadTask` which ignores the `httpBody` and
-        // instead takes the body payload as a parameter to add to the request.
-        // However in tests we're using OHHTTPStubs to stub out the HTTP upload.
-        // It has the known limitation that it doesn't simulate data upload,
-        // because the underlying protocol doesn't expose a hook for that.
-        // By setting `httpBody` here the data is still attached to the request,
-        // so OHHTTPStubs sees it.
-        // It shouldn't be too bad memory-wise and not duplicate the data in memory.
-        // This should only be a reference and Swift keeps track of all the places it's needed.
-        //
-        // See https://github.com/AliSoftware/OHHTTPStubs#known-limitations.
-        request.httpBody = data
-


I added that as part of the PingUploader code since it was part of the Glean implemenation, but we don't use OHHTTPStubs so it makes no sense to use here.

This is part of why we see those runtime warnings when running unit tests:

But even removing that request.httpBody = data, I was still seeing hundreds of The request of a upload task runtime warnings, which made me realize we call Glean telemetry without having it setup for unit tests in some test suite.

cyndichin · 2025-10-03T18:41:57Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift

-        Glean.shared.registerPings(GleanMetrics.Pings.shared)
-        Glean.shared.resetGlean(clearStores: true)


should we have setupTelemetry here as well? But I have a PR that should address the microsurvey ones appropriately since @isabelrios pinged me about it., maybe we can merge that one instead of using this since that's the path forward we want to go with? #29814

Yeah for sure, your PR is the way to go for that particular test suite! We can merge mine after yours is merged

thanks! I merged it and yoana has an upcoming one for ShareTelemetryTests, but this PR looks good to merge after resolving any conflicts. Appreciate you making the tickets 🙏

yoanarios · 2025-10-03T19:09:19Z

firefox-ios/firefox-ios-tests/Tests/ClientTests/Messaging/GleanPlumbMessageManagerTests.swift

 @testable import Client
 import MozillaAppServices

+// TODO: FXIOS-13565 - Migrate GleanPlumbMessageManagerTests to use mock telemetry or GleanWrapper


yoanarios

LGTM 🚀, thanks for creating all the follow up tickets

firefox-ios/Client/Telemetry/TelemetryWrapper.swift

mergify · 2025-10-07T13:04:55Z

This pull request has conflicts when rebasing. Could you fix it @lmarceau? 🙏

cyndichin

thanks @lmarceau for working on this and creating the tickets! looks good to me at high level, I haven't dived into the details, but feel free to merge when resolving conflicts since I trust @yoanarios approval!

github-actions · 2025-10-08T13:35:40Z

🚀 PR merged to main, targeting version: 144.1

lmarceau commented Oct 3, 2025

View reviewed changes

cyndichin reviewed Oct 3, 2025

View reviewed changes

lmarceau marked this pull request as ready for review October 3, 2025 18:53

lmarceau requested a review from a team as a code owner October 3, 2025 18:53

lmarceau requested review from Foxbolts, cyndichin and yoanarios and removed request for Foxbolts October 3, 2025 18:53

yoanarios reviewed Oct 3, 2025

View reviewed changes

yoanarios approved these changes Oct 3, 2025

View reviewed changes

lmarceau commented Oct 3, 2025

View reviewed changes

firefox-ios/Client/Telemetry/TelemetryWrapper.swift Outdated Show resolved Hide resolved

cyndichin reviewed Oct 7, 2025

View reviewed changes

lmarceau added 8 commits October 7, 2025 22:32

typo

32d822c

Fix this from the Glean protocol, we don't need this here

f17cb5a

Disable some unit tests + only call telemetry when needed

211783c

Fixes

a588121

Change order

23f3c93

Fix comments

e3cdb93

Fix todo comment

e4a636a

Minor comment fix

8ea301c

lmarceau force-pushed the lm/fix-telemetry branch from 2f68782 to 8ea301c Compare October 8, 2025 02:32

lmarceau merged commit 769c467 into main Oct 8, 2025
9 checks passed

lmarceau deleted the lm/fix-telemetry branch October 8, 2025 13:35

		Glean.shared.registerPings(GleanMetrics.Pings.shared)
		Glean.shared.resetGlean(clearStores: true)

Refactor FXIOS-13671 [Telemetry & Unit tests] Fix telemetry crashes in unit tests #29799

Refactor FXIOS-13671 [Telemetry & Unit tests] Fix telemetry crashes in unit tests #29799

Uh oh!

Conversation

lmarceau commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Tickets

💡 Description

Examples

Opinions

📝 Checklist

Uh oh!

mobiletest-ci-bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💪 Quality guardian

🧩 Neat Piece

💬 Description craftsman

✅ Per-file coverage

Client.app: Coverage: 37.21

Uh oh!

lmarceau Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

lmarceau Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

cyndichin Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

lmarceau Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyndichin Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

yoanarios Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

yoanarios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Oct 7, 2025

Uh oh!

cyndichin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lmarceau commented Oct 3, 2025 •

edited

Loading

mobiletest-ci-bot commented Oct 3, 2025 •

edited

Loading

lmarceau Oct 3, 2025 •

edited

Loading

cyndichin left a comment •

edited

Loading