[PM-27049] Initial version of the Test Harness app #2142
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2142 +/- ##
=======================================
Coverage 85.38% 85.39%
=======================================
Files 1726 1726
Lines 145626 145626
=======================================
+ Hits 124350 124352 +2
+ Misses 21276 21274 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Configs/Common-bwth.xcconfig
Outdated
| BASE_BUNDLE_ID = $(ORGANIZATION_IDENTIFIER).testharness | ||
| SHARED_APP_GROUP_IDENTIFIER = group.${ORGANIZATION_IDENTIFIER}.test-harness |
There was a problem hiding this comment.
🤔 Do we want these suffixes to mach? testharness vs test-harness?
There was a problem hiding this comment.
Nice catch! Indeed I'll fix that.
| /// The username field was updated. | ||
| case usernameChanged(String) | ||
|
|
||
| /// The password field was updated. | ||
| case passwordChanged(String) |
There was a problem hiding this comment.
⛏️ Does Claude know about our convention for alphabetizing cases, properties, functions?
There was a problem hiding this comment.
Yes, although sometimes it makes mistakes dunno why as it should be following the contributor's code styles https://contributing.bitwarden.com/contributing/code-style/swift#alphabetization.
I'll fix that.
|
Claude finished @fedemkr's task —— View job Code Review CompleteSummary of Changes Since Last ReviewThis PR introduces an initial Test Harness app for the Bitwarden iOS project. The app serves as a playground to test Bitwarden flows externally (Autofill, URL opening, etc.). The implementation follows Bitwarden's established architectural patterns with coordinator-based navigation, processor/state/action/effect patterns, and proper dependency injection via ServiceContainer. Recent commits addressed previous feedback including:
Critical Issues❌ Finding 5: Insufficient test coverage (28.57% patch coverage) The Codecov report shows only 28.57% patch coverage with 15 lines missing coverage. Per Bitwarden's testing guidelines (Testing.md), processors, services, and coordinators should have 80-100% test coverage. Missing tests:
Action Required: Add comprehensive test coverage following the patterns in Testing.md before merge. Example test structure neededFor class ScenarioPickerProcessorTests: BitwardenTestCase {
var subject: ScenarioPickerProcessor!
var coordinator: MockCoordinator<RootRoute, Void>!
override func setUp() {
super.setUp()
coordinator = MockCoordinator()
subject = ScenarioPickerProcessor(coordinator: coordinator.asAnyCoordinator())
}
func test_receive_scenarioTapped_withRoute_navigates() {
let scenario = ScenarioItem(id: "test", title: "Test", route: .simpleLoginForm)
subject.receive(.scenarioTapped(scenario))
XCTAssertEqual(coordinator.routes.last, .simpleLoginForm)
}
func test_receive_scenarioTapped_withoutRoute_doesNotNavigate() {
let scenario = ScenarioItem(id: "test", title: "Test", route: nil)
subject.receive(.scenarioTapped(scenario))
XCTAssertTrue(coordinator.routes.isEmpty)
}
}Minor FindingsFile is in 💭 Finding 2: Naming consistency question (Configs/Common-bwth.xcconfig:4) Bundle ID uses 📝 Finding 4: Incomplete DocC documentation (TestHarnessShared/Core/Platform/ServiceContainer.swift:20) Parameter documentation for ♻️ Finding 6: Missing mock generation setup While Suggested Improvements🎨 Consider adding snapshot tests for views Per Testing.md requirements, all user-facing views should have snapshot tests covering:
This would catch visual regressions as the Test Harness evolves. 🎨 Consider extracting scenario configuration Currently scenarios are defined inline in Good Practices Observed
Previous Review Comments Status✅ RESOLVED: @matt-livefront's comment about alphabetization - ⏳ PENDING: @matt-livefront's comment about naming consistency ( ⏳ ADDRESSED: @matt-livefront's comment about ⏳ ADDRESSED: @matt-livefront's comment about generated Localizations.swift - Action Items for Author
Security Considerations✅ No security concerns identified for this test harness app:
Compilation & Build Verification
Recommendation: Ensure CI builds pass before merge. |
There was a problem hiding this comment.
⛏️ I think this should be in a TestHelpers folder otherwise it's going to be included in the app itself.
There was a problem hiding this comment.
⛏️ I think we added this to a .gitignore in other targets.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) -> Bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
This file should be in a TestHelpers folder that's properly excluded from the main app target. Currently, it's at TestHarness/Application/TestHelpers/Support/TestingAppDelegate.swift, which suggests it will be included in the main app bundle.
Per the Testing.md guidelines, test helper files should be properly organized and excluded from the main app. The project-bwth.yml correctly excludes **/TestHelpers/* from the TestHarness target sources, but having this file in TestHelpers/Support/ may cause confusion.
Recommendation: Move this file to TestHarness/Application/TestHelpers/TestingAppDelegate.swift (remove the Support subdirectory) for clearer organization, or ensure the exclusion pattern is working as expected.
| CODE_SIGN_STYLE = Automatic | ||
| DEVELOPMENT_TEAM = LTZ2PFU5D6 | ||
| ORGANIZATION_IDENTIFIER = com.bitwarden | ||
| BASE_BUNDLE_ID = $(ORGANIZATION_IDENTIFIER).testharness |
There was a problem hiding this comment.
💭 Finding 2: Naming inconsistency between bundle ID and shared app group
The bundle ID uses testharness (no hyphen) while the comment from @matt-livefront previously noted this inconsistency. The shared app group also uses testharness.
For consistency with the PR branch name (test-harness-app) and typical naming conventions, consider whether test-harness would be more appropriate. However, if testharness is the intentional choice, this is acceptable.
Note: This was previously flagged by @matt-livefront. Has this been addressed or is testharness the final decision?
|
|
||
| /// The username field was updated. | ||
| case usernameChanged(String) | ||
| } |
There was a problem hiding this comment.
Per the Swift Code Style Guidelines, enum cases should be alphabetized. Currently:
passwordChanged(line 7)usernameChanged(line 10)
Should be:
| } | |
| /// Actions that can be processed by a `SimpleLoginFormProcessor`. | |
| /// | |
| enum SimpleLoginFormAction: Equatable { | |
| /// The password field was updated. | |
| case passwordChanged(String) | |
| /// The username field was updated. | |
| case usernameChanged(String) | |
| } |
Wait - they ARE already alphabetized (p before u). This appears to be correct.
Correction: After review, these cases are properly alphabetized. The previous review comment may have been about a different file or has since been fixed.
| /// Initialize a `ServiceContainer`. | ||
| /// | ||
| /// - Parameters: | ||
| /// - errorReportBuilder: A helper for building an error report containing the details of an |
There was a problem hiding this comment.
📝 Finding 4: Incomplete DocC documentation
The DocC comment for the init method is incomplete. The parameter description cuts off mid-sentence:
/// - errorReportBuilder: A helper for building an error report containing the details of anShould be completed to something like:
| /// - errorReportBuilder: A helper for building an error report containing the details of an | |
| /// Initialize a `ServiceContainer`. | |
| /// | |
| /// - Parameters: | |
| /// - errorReportBuilder: A helper for building an error report containing the details of an error that occurred. | |
| public init( |
|
Warning @fedemkr Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
🎟️ Tracking
PM-27049
📔 Objective
Build a new Test Harness app to act as a playground to fire Bitwarden flows externally like Autofill or opening URLs.
📸 Screenshots
Scenarios
Simple Login Form
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes