Skip to content

Refactor FXIOS-13740 [Unit Tests] Microsurvey Telemetry Tests #29814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 6, 2025
Merged

Refactor FXIOS-13740 [Unit Tests] Microsurvey Telemetry Tests #29814

merged 3 commits into from
Oct 6, 2025

Conversation

@cyndichin
Copy link
Contributor

@cyndichin cyndichin commented Oct 3, 2025 •
edited
Loading

📜 Tickets

Jira ticket
Github issue

💡 Description

Fix tests failing frequently in pipeline: testSurveyDidAppearAction() reported by Isabel

Pipeline: https://app.bitrise.io/app/6c06d3a40422d10f/pipelines/a4d07818-1e61-40b3-8a58-0098e53faa4b?tests_filter_status=failed&tab=tests

Following the guidelines listed here:
image

cc: @lmarceau @ih-codes @dataports since we discussed this in our previous unit test meeting.

📝 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

@cyndichin cyndichin requested a review from a team as a code owner October 3, 2025 18:34
@cyndichin cyndichin requested a review from yoanarios October 3, 2025 18:34
@cyndichin cyndichin mentioned this pull request Oct 3, 2025
Merged
7 tasks
@lmarceau
Copy link
Contributor

lmarceau commented Oct 3, 2025

Are those guidelines from the screenshot in a wikipage somewhere? I think they should be on this page right?

@cyndichin cyndichin force-pushed the cc/FXIOS-13740_fix-test-survey-did-action-appear branch from 62283de to b001d59 Compare October 3, 2025 18:59
@cyndichin
Copy link
Contributor Author

cyndichin commented Oct 3, 2025

Are those guidelines from the screenshot in a wikipage somewhere? I think they should be on this page right?

It's from Slack currently, but I'll probably write one after we finalize this PR so we have some proper examples along with the guidelines!

cyndichin
cyndichin commented Oct 3, 2025
View reviewed changes
firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyTelemetryTests.swift

XCTAssertEqual(mockGleanWrapper.recordEventCalled, 1)
XCTAssertEqual(savedExtras.surveyId, expectedSurveyId)
XCTAssert(savedMetric === event, "Received \(savedMetric) instead of \(event)")
Copy link
Contributor Author

@cyndichin cyndichin Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change here from testing other telemetry structs with the mock is I removed the type comparison as well as the previous debug message.

ih-codes
ih-codes reviewed Oct 3, 2025
View reviewed changes
Copy link
Collaborator

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is a great example of following the new telemetry testing practices we've discussed! 🥇

(I was just about to mention about the telemetry test type check, but then I saw you commit update to check on actual object vs types so all good there!)

Just one suggestion for the mock implementation to improve the tests further. 😁

@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented Oct 3, 2025 •
edited
Loading

Messages
📖 Project coverage: 38.57%

💪 Quality guardian

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

🥇 Perfect PR size

Smaller PRs are easier to review. Thanks for making life easy for reviewers! ✨

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

Client.app: Coverage: 37.34

File Coverage
MicrosurveyMiddleware.swift 100.0% ✅
MicrosurveyTelemetry.swift 100.0% ✅

Generated by 🚫 Danger Swift against 16226bb

@cyndichin cyndichin requested a review from ih-codes October 6, 2025 12:30
@cyndichin
Copy link
Contributor Author

cyndichin commented Oct 6, 2025

@ih-codes thanks for reviewing and always being so detailed oriented ❤️ , I updated the PR. Let me know if you have any other nits / comments. Thanks! :)

yoanarios
yoanarios approved these changes Oct 6, 2025
View reviewed changes
Copy link
Contributor

@yoanarios yoanarios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 🚀

ih-codes
ih-codes approved these changes Oct 6, 2025
View reviewed changes
Copy link
Collaborator

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this!! 🔥

@cyndichin cyndichin merged commit c1435af into main Oct 6, 2025
8 checks passed
@cyndichin cyndichin deleted the cc/FXIOS-13740_fix-test-survey-did-action-appear branch October 6, 2025 16:17
@cyndichin
Copy link
Contributor Author

cyndichin commented Oct 13, 2025

Are those guidelines from the screenshot in a wikipage somewhere? I think they should be on this page right?

Updated wiki here - https://github.com/mozilla-mobile/firefox-ios/wiki/Glean-testing#general-testing-telemetry-guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clarmso clarmso clarmso approved these changes

@yoanarios yoanarios yoanarios approved these changes

@ih-codes ih-codes ih-codes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cyndichin @lmarceau @mobiletest-ci-bot @clarmso @yoanarios @ih-codes