Skip to content
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.

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

Refactor FXIOS-11025 [Glean improvements] Refactor HomepageTelemetry to Use Glean Wrapper for Improved Testability 🚀 #24121

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

krishpranav
Copy link

@krishpranav krishpranav commented Jan 14, 2025

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR refactors the HomepageTelemetry class to inject a custom GleanWrapper for better dependency management and to facilitate mocking in unit tests. The current approach directly interacts with Glean metrics, making it difficult to test and mock. By introducing a GleanWrapperProtocol and a CustomGleanWrapper class, we decouple the telemetry logic from Glean, allowing for more flexible and testable code. 🔧

Key Changes:

  • GleanWrapperProtocol: Defines a protocol for interacting with Glean's telemetry methods. 📜
  • CustomGleanWrapper: Implements the protocol, acting as the bridge to Glean's API for telemetry recording. 🧰
  • HomepageTelemetry: Updated to inject the GleanWrapperProtocol, making it more modular and easier to mock for unit tests. 🔄

Additionally, I have added unit tests to verify the correct functionality of the sendHomepageTappedTelemetry method, ensuring that the proper telemetry data is being recorded based on the input values. These tests check both private and normal mode toggle scenarios. 🧪

Output Log:

  • testPrivateModeShortcutToggleTappedInNormalMode:
    Test Passed: Metric recorded with isPrivateMode: true ✅
    
  • testPrivateModeShortcutToggleTappedInPrivateMode:
    Test Passed: Metric recorded with isPrivateMode: false ✅
    

Test Cases:

  • testPrivateModeShortcutToggleTappedInNormalMode: Verifies that the telemetry records "true" for the isPrivateMode flag when entering normal mode. 🔒
  • testPrivateModeShortcutToggleTappedInPrivateMode: Verifies that the telemetry records "false" for the isPrivateMode flag when entering private mode. 🕵️

Issue:

📝 Checklist

You have to check all boxes before merging ✅

  • Filled in the above information (tickets numbers and description of your work) 📝
  • Updated the PR name to follow our [PR naming guidelines](https://github.com/mozilla-mobile/firefox-ios/wiki/Pull-Request-Naming-Guide) 📚
  • Wrote unit tests and/or ensured the tests suite is passing 🧑‍💻
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver) ♿
  • If needed, I updated documentation / comments for complex code and public methods 📑
  • If needed, added a backport comment (example @Mergifyio backport release/v120) 🔄

@krishpranav krishpranav requested a review from a team as a code owner January 14, 2025 11:59
@krishpranav krishpranav requested a review from dataports January 14, 2025 11:59
@lmarceau lmarceau changed the title Refactor HomepageTelemetry to Use Glean Wrapper for Improved Testability 🚀 Refactor FXIOS-11025 [Glean improvements] Refactor HomepageTelemetry to Use Glean Wrapper for Improved Testability 🚀 Jan 14, 2025
Copy link
Contributor

@lmarceau lmarceau 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 the change! HomepageTelemetry looks great. We just miss to adjust the unit tests HomepageTelemetryTests and all should be good 👍

@krishpranav
Copy link
Author

@lmarceau, i've fixed the test cases. kindly check it.

@DianQK
Copy link

DianQK commented Jan 16, 2025

Note that this user appears to be identity farming with AI generated content

rust-lang/rust#135436
rust-lang/stdarch#1701
Alamofire/Alamofire#3924
Andino-Labs/yiqi-mobile#4
facebook/react-native#48505

Additional scrutiny of contributions is warranted.

@krishpranav
Copy link
Author

@DianQK please dont keep false accusations like these. I noticed the concerns raised regarding my contributions, and I would like to clarify that I am committed to maintaining integrity and transparency in all of my work. I take pride in delivering quality contributions and am always open to further discussion or clarification on any matter.

@krishpranav krishpranav requested a review from lmarceau January 19, 2025 06:39
@krishpranav krishpranav requested a review from lmarceau January 21, 2025 07:01
Copy link
Contributor

@lmarceau lmarceau left a comment

Choose a reason for hiding this comment

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

Ill run the build and if green then we can merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Glean Wrapper for HomepageTelemetry
3 participants