Skip to content

[MOB-4320] Unit tests compilation fixes [reopened]#1128

Open
d4r1091 wants to merge 16 commits intomainfrom
ls-mob-4320-unit-tests-compilation-fix
Open

[MOB-4320] Unit tests compilation fixes [reopened]#1128
d4r1091 wants to merge 16 commits intomainfrom
ls-mob-4320-unit-tests-compilation-fix

Conversation

@d4r1091
Copy link
Copy Markdown
Member

@d4r1091 d4r1091 commented May 4, 2026

MOB-4320

Context

After the v147 Firefox upstream upgrade, unit tests fail to compile. The main root cause is Swift 6 strict concurrency enforcement: in Swift 6, SWIFT_STRICT_CONCURRENCY = minimal (set during the Tuist migration) has no effect — strict concurrency is unconditionally enforced, unlike Swift 5 where it acted as a suppression mechanism.`

A previous partial Swift 6 migration addressed ~30/35 production source files but explicitly deferred test file migration to unblock the v147 upgrade. Additionally, the v147 bulk file replacement overwrote several individually cherry-picked upstream commits that had already fixed Swift 6 warnings in test files (e.g., RustRemoteTabsTests, TestBrowserDB, WebServer.swift closures). Cherry-picking those fixes back was not viable due to source conflicts from the upgrade.

Approach

Investigation

  • Confirmed that SWIFT_STRICT_CONCURRENCY = minimal is a no-op in Swift 6 and cannot suppress the errors
  • Attempted cherry-picking upstream concurrency fixes — all conflicted due to v147 source changes
  • Compared concurrency fix patterns with upstream Firefox iOS to ensure alignment

Fix Patterns Applied (aligned with upstream)

The following Swift 6 concurrency patterns were applied consistently across test and mock files:

  1. @MainActor on test classes — for tests interacting with main-actor-isolated APIs
  2. @unchecked Sendable — on non-final test/mock classes conforming to Sendable protocols where full conformance isn't feasible
  3. nonisolated(unsafe) var — for mutable variables captured across isolation boundaries in closures
  4. MainActor.assumeIsolated { } — for accessing @MainActor properties within synchronous callbacks known to run on the main thread
  5. Extracting @MainActor properties before autoclosure — to avoid isolation violations in XCTAssert* calls

Other fixes

  • Deleted orphaned test files referencing APIs removed in v147 (Fakespot, legacy Homepage, FeatureSwitch, etc.)
  • Updated mock files to match v147 protocol changes (MockProfile, MockTabManager, MockGleanWrapper, MockLogger, MockFiles, etc.)
  • Fixed Tuist target dependencies so test targets resolve their imports correctly
  • Replaced the .xctestplan file with fully inline test target and skip declarations in Schemes+Ecosia.swift using Tuist's skippedTests parameter — Tuist resolves target identifiers at generation time, eliminating stale UUID issues that occurred whenever the project was regenerated
  • Updated CI to Xcode 26.3, macOS 26 runner, and iPhone 17 simulator
  • Switched Danger gem source from SSH to HTTPS for CI compatibility

Notes

  • Upstream Firefox iOS mostly applies the same concurrency patterns (confirmed for example via FXIOS-15180 and FXIOS-14162), so hopefully these fixes will not conflict much with future upstream merges
  • Even if building for tests now work with no compilation error and we can run them - tests are still failing which will be tackled separately.

CI: Temporarily disabled Merge Unit Tests

The Merge Unit Tests workflow (.github/workflows/merge_tests.yml) has been temporarily disabled by clearing its trigger branches. Running them as a merge gate at this stage produces noise rather than signal and adds significant overhead (~30+ min) with no actionable feedback.

Fixing issues and re-enabling them has been split into MOB-4384.

Before merging

Checklist

  • I performed some relevant testing on a real device and/or simulator for both iPhone and iPad
  • I wrote Unit Tests that confirm the expected behaviour
  • I updated only the english localization source files if needed
  • I added the // Ecosia: helper comments where needed
  • I made sure that any change to the Analytics events included in PR won't alter current analytics (e.g. new users, upgrading users)
  • I included documentation updates to the coding standards or Confluence doc, when needed

lucaschifino and others added 16 commits April 13, 2026 15:13
These files still exist in Firefox upstream (with Swift 6 @mainactor
patterns applied) and cover core tab management, search, toolbar state,
and JumpBackIn functionality used by Ecosia.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Replaces 13 individual file paths with wildcards for Mocks/,
DependencyInjection/, and Coordinators/Mocks/ so new mock files are
picked up automatically. Directories that also contain test classes
are kept as specific paths to avoid duplicating tests into EcosiaTests.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
All 5 files restored from upstream cannot compile against Ecosia's current
production APIs (removed inactive tabs, missing mock properties, upstream-only
Nimbus features, renamed AppState fields). They were correctly deleted in the
original compilation fix and remain deleted.

Rename private mock types in EcosiaWebViewModalTests to avoid name conflicts
introduced by the Mocks/*.swift wildcard (MockCoordinator, MockNavigationAction,
MockFrameInfo → EcosiaWebView-prefixed variants).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@d4r1091 d4r1091 requested a review from a team May 4, 2026 16:20
Copy link
Copy Markdown
Collaborator

@lucaschifino lucaschifino left a comment

Choose a reason for hiding this comment

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

@d4r1091 I guess you can't review it now that it's been reopened on your name 😅

From my side it's all good (well, I was the one implementing anyway 🙈) - it might be cleaner to merge this if you agree (or make any small adjustments you see) and then continue the work on the next ticket anyway.

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.

2 participants