[MOB-3113] Add Ecosia Framework, remove Core and update tests

[d4r1091]

MOB-3113

Context

Original work #835

This PR is the result of a series of improvements in the overall handling of:

• Unit Tests
• Snapshot Tests
• Repository size
• Core removal
• Creation of the Ecosia Framework
• Include that as part of the Upgrade work

Approach

Consider everything we did as part of #835 and review it in favor of the major improvement for Snapshot Testing 📸 .

As further context, we explored the possibility of implementing Git LFS to improve how we handle the raw snapshots' artifacts.
Specifically, we wanted to remove all these artifacts from the main repo.
Git LFS seemed the perfect solution to implement right away.

Unfortunately, this option would not fit into our project structure because:

• we are implementing LFS on a public forked repo
• we do not have write access to the root repo we forked from
• LFS is not implemented by the root repo in the first place
  Source

However, option two, which quickly became available, consisted of implementing submodules.
We all know how quickly things may become messy with submodules, but it is worth a try for this precise scenario of purely serving resources and not code.

The first challenge was to include the submodule only for *.png artifacts as we didn't want to make another source for our code (as we challenged ourselves to solve this in the first place porting Core after all 😅 ).

The way the Snapshot Testing library works https://github.com/pointfreeco/swift-snapshot-testing is to make a __Snapshots__ subfolder at each of the paths our tests live.
To make submodules work, we wanted to decouple the artifact source of truth from the test domains.
Fortunately, the library provides us with a not-so-straightforward solution for that, eventually defining a custom directory while keeping our standard folder layout.

GIt LFS implementation

With the submodule challenge sorted out, going the extra mile and further improving the newly created submodule felt almost like a no-brainer.
Therefore, I actioned it.

By doing so, we are maximizing effort and resources when it comes to performing such tests while keeping the repos as lightweight as possible.

Other

We will need to monitor the Git LFS billing as per the repo size, but in the worst-case scenario, we can always roll back Git LFS and deal with raw artifacts, while keeping them in the submodule.

Before merging

Checklist

• I performed some relevant testing on a real device and/or simulator
• I wrote Unit Tests that confirm the expected behaviour
• I added the // Ecosia: helper comments where needed
• I included documentation updates to the coding standards or Confluence doc, when needed

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The test testDoesNotRegisterForAdNetworkGivenAleradyDonePreviously might not be correctly validating the scenario where registration has already occurred. Ensure that the test setup and assertions align with the intended logic. func testDoesNotRegisterForAdNetworkGivenAleradyDonePreviously() async throws { objectPersister.setValueFor(.firstSkanCallTimestamp, value: 1) XCTAssertNil(MockSkan.conversionValue) XCTAssertNil(objectPersister.getValueFor(.lastSkanCallTimestamp)) XCTAssertNil(objectPersister.getValueFor(.conversionValue)) try await helper.registerAppForAdNetworkAttribution() XCTAssertNil(MockSkan.conversionValue) XCTAssertNil(objectPersister.getValueFor(.lastSkanCallTimestamp)) XCTAssertNil(objectPersister.getValueFor(.conversionValue)) XCTAssertNil(objectPersister.getValueFor(.previousFineValue)) XCTAssertNil(objectPersister.getValueFor(.coarseValue(window: .first))) XCTAssertNil(objectPersister.getValueFor(.previousCoarseValue(window: .first))) XCTAssertNil(objectPersister.getValueFor(.windowLockTimestamp(window: .first))) Performance Concern The save method in the User class writes to disk on every property change. This could lead to performance issues due to frequent I/O operations. Consider batching updates or using a debounce mechanism. private func save() { let user = self User.queue.async { try? JSONEncoder().encode(user).write(to: FileManager.user, options: .atomic) } Test Coverage The testReceivingUpdatesCounterOnlyIncrease test assumes specific behavior for counter updates. Ensure edge cases, such as concurrent updates or invalid cookie values, are also tested. func testReceivingUpdatesCounterOnlyIncrease() { User.shared.searchCount = 5 extractReceivedCookies([HTTPCookie(properties: [.name: "ECFG", .domain: ".ecosia.org", .path: "/", .value: "as=0:f=s:mc=en-uk:t=4"])!]) XCTAssertEqual(5, User.shared.searchCount) extractReceivedCookies([HTTPCookie(properties: [.name: "ECFG", .domain: ".ecosia.org", .path: "/", .value: "as=0:f=s:mc=en-uk:t=6"])!]) XCTAssertEqual(6, User.shared.searchCount)

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add error handling for URL creation Add error handling for the forced unwrapping of URLs using URL(string:) to prevent potential runtime crashes if the URL string is invalid. firefox-ios/Ecosia/Core/Environment/URLProvider.swift [15] -return URL(string: "https://www.ecosia.org")! +guard let url = URL(string: "https://www.ecosia.org") else { + fatalError("Invalid URL string") +} +return url Suggestion importance[1-10]: 9 __ Why: Adding error handling for URL creation is crucial to prevent runtime crashes caused by invalid URL strings. The suggestion is accurate and improves the robustness of the code.	High
	Add error handling for JSON decoding Add error handling for decoding the response data to ensure the application doesn't crash if the data format is invalid. firefox-ios/Ecosia/Core/FeatureManagement/Unleash/UnleashFeatureManagementSessionInitializer.swift [49-53] -guard let remoteToggles = try? JSONDecoder().decode(Unleash.FeatureResponse.self, from: data) else { - return model as? T +do { + let remoteToggles = try JSONDecoder().decode(Unleash.FeatureResponse.self, from: data) + model.toggles = Set(remoteToggles.toggles) +} catch { + throw UnleashFeatureManagementSessionInitializer.Error.noData } +return model as? T Suggestion importance[1-10]: 9 __ Why: Adding error handling for JSON decoding ensures the application can gracefully handle invalid or unexpected data formats, improving its robustness. The suggestion is accurate and beneficial.	High
	Fix missing superclass initializer call Ensure that the required init?(coder:) initializer calls its superclass initializer to avoid potential runtime issues when decoding the button. firefox-ios/Client/Ecosia/UI/EcosiaPrimaryButton.swift [15] -required init?(coder: NSCoder) { nil } +required init?(coder: NSCoder) { + super.init(coder: coder) +} Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential issue where the required initializer does not call its superclass initializer, which could lead to runtime errors. Fixing this ensures proper initialization of the UIButton.	Medium
	Add error handling for event tracking Ensure that the track function properly handles cases where tracker.track(event) might fail or return an error, as this could lead to unhandled exceptions or silent failures. firefox-ios/Ecosia/Analytics/Analytics.swift [41-43] private func track(_ event: SnowplowTracker.Event) { guard User.shared.sendAnonymousUsageData else { return } - _ = tracker.track(event) + do { + try tracker.track(event) + } catch { + // Handle or log the error appropriately + } } Suggestion importance[1-10]: 8 __ Why: Adding error handling for the track function is a significant improvement as it ensures that potential errors during event tracking are managed properly, preventing silent failures and improving the robustness of the analytics system.	Medium
	Add validation for cookie creation Validate that HTTPCookie creation in makeIncognitoCookie and makeStandardCookie does not fail silently, as this could lead to runtime crashes or unexpected behavior. firefox-ios/Ecosia/Core/Cookie.swift [80-86] -public static func makeIncognitoCookie(_ urlProvider: URLProvider = Environment.current.urlProvider) -> HTTPCookie { - HTTPCookie(properties: [.name: Cookie.main.name, - .domain: ".\(urlProvider.domain ?? "")", - .path: "/", - .value: Cookie.incognitoValues.map { $0.0 + "=" + $0.1 }.joined(separator: ":")])! +public static func makeIncognitoCookie(_ urlProvider: URLProvider = Environment.current.urlProvider) -> HTTPCookie? { + guard let cookie = HTTPCookie(properties: [.name: Cookie.main.name, + .domain: ".\(urlProvider.domain ?? "")", + .path: "/", + .value: Cookie.incognitoValues.map { $0.0 + "=" + $0.1 }.joined(separator: ":")]) else { + // Log or handle the failure to create a cookie + return nil + } + return cookie } Suggestion importance[1-10]: 8 __ Why: Validating the creation of HTTPCookie objects is crucial to avoid runtime crashes or unexpected behavior, especially when the cookie creation fails. This improvement enhances the stability and reliability of the code.	Medium
	Ensure thread safety for shared resources Ensure thread safety when accessing or modifying the rules array by using a synchronization mechanism like a DispatchQueue. firefox-ios/Ecosia/Core/FeatureManagement/Unleash/Unleash+RefreshComponent.swift [16-18] -if !rules.contains(where: { type(of: $0) == type(of: rule) }) { - rules.append(rule) +Self.queue.sync { + if !rules.contains(where: { type(of: $0) == type(of: rule) }) { + rules.append(rule) + } } Suggestion importance[1-10]: 8 __ Why: Ensuring thread safety for the rules array is important as it is a shared resource. The suggestion is valid and improves the code's reliability in concurrent environments.	Medium
	Validate theme parameter before use Validate the theme parameter in the configure method to ensure it is not nil before applying it, to prevent potential runtime errors. firefox-ios/Client/Ecosia/UI/NTP/Impact/NTPImpactCell.swift [78] -applyTheme(theme: theme) +if let theme = theme { + applyTheme(theme: theme) +} Suggestion importance[1-10]: 7 __ Why: The suggestion to validate the theme parameter before applying it is valid and helps prevent potential runtime errors if theme is unexpectedly nil. This improves the robustness of the code.	Medium
	Add timeout to prevent blocking Add a timeout mechanism to the parseBookmarks method to prevent potential indefinite blocking if the dispatchQueue fails to complete the task. firefox-ios/Ecosia/Core/Bookmarks/BookmarkParser.swift [28-41] public func parseBookmarks() async throws -> [BookmarkItem] { try await withCheckedThrowingContinuation { continuation in + let timeout = DispatchTime.now() + .seconds(10) + dispatchQueue.asyncAfter(deadline: timeout) { + continuation.resume(throwing: BookmarkParserError.cancelled) + } dispatchQueue.async { [weak self] in guard let self = self else { return continuation.resume(throwing: BookmarkParserError.cancelled) } do { let result = try self.parse(element: try self.document.getLeadingDL()) continuation.resume(with: .success(result)) } catch { continuation.resume(throwing: error) } } } } Suggestion importance[1-10]: 7 __ Why: Introducing a timeout mechanism in the parseBookmarks method is a valuable enhancement to prevent indefinite blocking, ensuring better reliability and responsiveness of the application.	Medium
General	Handle errors in network requests Add error handling for the dataTask completion handler to manage potential errors during the network request. firefox-ios/Ecosia/Core/Images.swift [39-47] -session.dataTask(with: url) { data, _, _ in +session.dataTask(with: url) { data, _, error in DispatchQueue.main.async { [weak self] in + if let error = error { + print("Failed to download image: \(error)") + return + } data.map { let item = Item(url, $0) self?.send(item) self?.items.insert(item) } } }.resume() Suggestion importance[1-10]: 8 __ Why: Adding error handling for the dataTask completion handler is important to manage potential network errors effectively. The suggestion is valid and enhances the code's reliability.	Medium
	Optimize memory usage in serialization Ensure that the serializeBookmarks method handles potential memory issues when processing a large number of bookmarks, as this could lead to performance degradation or crashes. firefox-ios/Ecosia/Core/Bookmarks/BookmarkSerializer.swift [23-51] public func serializeBookmarks(_ bookmarks: [BookmarkItem]) async throws -> String { try await withCheckedThrowingContinuation { continuation in dispatchQueue.async { [weak self] in guard let self = self else { return continuation.resume(throwing: BookmarkSerializerError.cancelled) } /// The trailing open <p> tag is part of the Netscape Bookmark file syntax var html = """ <!DOCTYPE NETSCAPE-Bookmark-file-1> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=UTF-8"> <Title>Bookmarks</Title> <H1>Bookmarks</H1> <DL><p> """ for bookmark in bookmarks { - html += self.bookmarkBody(for: bookmark, indentation: 2) + autoreleasepool { + html += self.bookmarkBody(for: bookmark, indentation: 2) + } } html += """ \(String.indent(by: 1))</DL><p> </HTML> """ continuation.resume(returning: html) } } } Suggestion importance[1-10]: 6 __ Why: Using autoreleasepool to manage memory during the serialization of a large number of bookmarks is a reasonable optimization. While not critical, it helps prevent potential memory issues and improves performance for large datasets.	Low

[lucaschifino]

Awesome job bringing this to the upgrade 👏 `Also great to see gitmodules worked out for snapshot, I agree it's a big improvement 🎉

Most of the review #816 is persistent and looks good to me!

Added a couple minor comments, the one about CD might be more relevant? Also waiting for the slack discussion about security to resolve before approving.

[lucaschifino]

LGTM 🚀