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

[MOB-2979] Analytics integration tests continuation #810

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

Conversation

d4r1091
Copy link
Member

@d4r1091 d4r1091 commented Nov 8, 2024

MOB-2979

Context

We started and implemented the first batch of Analytics integration tests.
This PR wants to enrich its content of.

If you want to utilize this PR and finish all the remaining ones, can do 👍

Approach

Some "accessors stretch" was made to accommodate the testing.
Despite some enums were CaseIterable, I was unable to access to the allCases so I ended up re-adding them into the tests.

Added the following tests

  • "Learn" more tap on the About Ecosia section
  • Top site actions
  • Multiply Impact screens (referrals)
  • News items and detail screen view
  • The whole onboarding flow

For the nasty issue with linkCopying test, I realized there is something wrong with the .touchUpdInside event only when run in CI. I removed it for now as we may find another solution later. I didn't want to keep the test but skipping it as it could be easily missed so when continuing with Analytics Integration Test we can find it still as "to to".
My suggestion is to review that component altogether and make it as a UIButton so that we can guarantee reliability. If you agree I can make a ticket out of it.

Other

  • It also aims to standardize the comments throughout the testing functions
  • Fixes some linting issues

Before merging

Checklist

  • I performed some relevant testing on a real device and/or simulator
  • I wrote Unit Tests that confirm the expected behavior

@d4r1091 d4r1091 requested a review from a team November 8, 2024 15:44
Copy link

github-actions bot commented Nov 8, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Access Control
The access level of copyControl, inviteButton, and learnMoreButton has been changed to private(set). Ensure that this change is intentional and justified, as it exposes these controls for modification from outside the class, which might not be desirable.

Access Control
The learnMoreButton has been changed to private(set). Review if this exposure is necessary and ensure it aligns with the encapsulation principles of the class.

Access Control
The collection property's access level has been changed to private(set). Verify if this change is required for testing purposes or other valid reasons, and consider the implications of broader access.

Copy link

github-actions bot commented Nov 8, 2024

PR Code Suggestions ✨

No code suggestions found for the PR.

@d4r1091 d4r1091 force-pushed the dc-mob-2979-analytics-integration-test-continuation branch from 4b2e53a to a0615d2 Compare November 11, 2024 14:09
# Conflicts:
#	EcosiaTests/Analytics/AnalyticsSpyTests.swift
@d4r1091 d4r1091 force-pushed the dc-mob-2979-analytics-integration-test-continuation branch from aef453c to 342938e Compare November 12, 2024 23:55
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.

1 participant