MBL-1017: Add features to CombineTestObserver #1920
Conversation
| } | ||
| } | ||
|
|
||
| public func assertDidComplete(_ message: String = "Should have completed.", |
There was a problem hiding this comment.
All these asserts are, for the most part, copy-pasted.
| case .finished: | ||
| return true | ||
| case .error: | ||
| return true |
There was a problem hiding this comment.
This is one difference. A failed publisher in Combine is 'dead' and complete, whereas I believe that's not true in ReactiveSwift.
amy-at-kickstarter
force-pushed
the
feat/adyer/mbl-1017/combine-test-observer
branch
from
f633559
to
0ac46ff
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
- Coverage 83.75% 83.75% -0.01%
==========================================
Files 1223 1224 +1
Lines 111784 111902 +118
Branches 29734 29799 +65
==========================================
+ Hits 93623 93719 +96
- Misses 17140 17149 +9
- Partials 1021 1034 +13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks good! I'm a little concerned about the different behaviors surrounding failures - for example, if we're waiting for updates from a database and we have a network error, we'd probably not want to stop subscribing to that database; we'd just want to wait for the next update. I'm guessing there are ways to handle that, but it might require more refactoring. Do you know if there is an obvious way to use combine if we want to handle errors differently? Either way, useful callout!
|
|
||
| assert( | ||
| errors.count <= 1, | ||
| "I'm pretty sure a Combine publisher can only ever emit one error. If this fails, we've learned something new today." |
There was a problem hiding this comment.
Nit: Do we want this check in our codebase? I'm okay with it since I'm kind of curious, but it feels a bit unprofessional/informal.
There was a problem hiding this comment.
Ha, I like throwing a little informality into code sometimes, but if you want me to make the language more neutral I definitely can.
There was a problem hiding this comment.
I'm used to an "avoid we" policy and I do have a slight preference for more neutral language, but I also think this is kind of fun and it's still clear who "we" is. So up to you.
There was a problem hiding this comment.
Interesting point on "we"! That's one I hadn't heard before.
| _ message: String = "Should have terminated, i.e. completed/failed/interrupted.", | ||
| file: StaticString = #file, line: UInt = #line | ||
| ) { | ||
| XCTAssertTrue(self.didFail || self.didComplete, message, file: file, line: line) |
There was a problem hiding this comment.
Nit: I don't think you need self.didFail here - if it failed self.didComplete will return true as well. Same for didNotTerminate below
There was a problem hiding this comment.
Legit! This is copy-pasted from the original TestObserver so I'm inclined to just leave it for parity.
Yeah, this is something I noticed early on. I added a helper method for this case specifically: https://github.com/kickstarter/ios-oss/blob/main/KsApi/Publisher%2BService.swift#L40 |
amy-at-kickstarter
deleted the
feat/adyer/mbl-1017/combine-test-observer
branch
📲 What
This makes
CombineTestObserverequivalent toTestObserver. Heck, I even just copy-pasted some code over, because it worked fine!🤔 Why
When we write new view models in Combine, we'll want to test them. This means that, if we're porting code, we can re-use the same (or write very similar) tests.