Add MarkOrderAsReadUseCase tests #13946
Conversation
|
|
| @@ -24,7 +24,7 @@ final class NoteListMapperTests: XCTestCase { | |||
| /// Verifies that all of the Sample Notifications are properly parsed. | |||
| /// | |||
| func test_sample_notifications_are_properly_decoded() { | |||
| XCTAssertEqual(sampleNotes.count, 44) | |||
| XCTAssertEqual(sampleNotes.count, 45) | |||
There was a problem hiding this comment.
Changed since the notifications-load-all.json file has added notes
| @@ -40,7 +40,7 @@ final class NotificationsRemoteTests: XCTestCase { | |||
| XCTAssertTrue(result.isSuccess) | |||
|
|
|||
| let notes = try result.get() | |||
| XCTAssertEqual(notes.count, 44) | |||
| XCTAssertEqual(notes.count, 45) | |||
There was a problem hiding this comment.
Changed since the notifications-load-all.json file has added notes
| @@ -7671,6 +7671,54 @@ | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "id": 100041, | |||
There was a problem hiding this comment.
Unread note with order id
| @@ -49,7 +49,7 @@ class NotificationStoreTests: XCTestCase { | |||
| XCTAssertEqual(viewStorage.countObjects(ofType: Storage.Note.self), 0) | |||
| let action = NotificationAction.synchronizeNotifications() { (error) in | |||
| XCTAssertNil(error) | |||
| XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 44) | |||
| XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 45) | |||
There was a problem hiding this comment.
Changed since the notifications-load-all.json file has added notes
| @@ -111,7 +111,7 @@ class NotificationStoreTests: XCTestCase { | |||
| /// Initial Sync | |||
| /// | |||
| let initialSyncAction = NotificationAction.synchronizeNotifications() { (error) in | |||
| XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 44) | |||
| XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 45) | |||
There was a problem hiding this comment.
Changed since the notifications-load-all.json file has added notes
| "id": 100042, | ||
| "note_hash": 987654, | ||
| "type": "store_order", | ||
| "read": true, |
There was a problem hiding this comment.
Read note with order id
|
|
||
| @MainActor | ||
| func test_markOrderNoteAsReadIfNeeded_with_network_unreadNote() async { | ||
| let expectation = self.expectation(description: "Mark order as read with network") |
There was a problem hiding this comment.
This is just a style preference, but I find this kind of expectation-based test a bit harder to read than one using e.g. waitForAsync. For example:
@MainActor
func test_markOrderNoteAsReadIfNeeded_with_network_unreadNote() async {
guard let note = self.sampleNote(read: false), let orderID = note.meta.identifier(forKey: .order) else {
return XCTFail()
}
let result = await waitForAsync { promise in
self.network.simulateResponse(requestUrlSuffix: "notifications", filename: "notifications-load-all")
self.network.simulateResponse(requestUrlSuffix: "notifications/read", filename: "generic_success")
promise(await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(network: self.network, noteID: note.noteID, orderID: orderID))
}
switch result {
case .success(let markedNoteID):
XCTAssertEqual(note.noteID, markedNoteID)
case .failure(let error):
XCTFail(error.localizedDescription)
}
}Anyway, no need to change the test; I just wanted to offer that as an alternative.
There was a problem hiding this comment.
Picking up from @rachelmcr 's observation, I believe it should be possible to do away entirely with custom code do manage the async code.
@MainActor
func test_markOrderNoteAsReadIfNeeded_with_network_unreadNote() async throws {
let note = try XCTUnwrap(sampleNote(read: false))
let orderID = try XCTUnwrap(note.meta.identifier(forKey: .order))
network.simulateResponse(requestUrlSuffix: "notifications", filename: "notifications-load-all")
network.simulateResponse(requestUrlSuffix: "notifications/read", filename: "generic_success")
let result = await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(network: network, noteID: note.noteID, orderID: orderID)
switch result {
case .success(let markedNoteID):
XCTAssertEqual(note.noteID, markedNoteID)
case .failure(let error):
XCTFail(error.localizedDescription)
}
}This code seems better to me because:
- Uses no custom code, just Swift's
async / awaitand XCTest assertions - Has a one level indentation
- By removing closures, also removes the cumbersome
self.accesses
_Works on my machine_™
There was a problem hiding this comment.
None of my comments affect what the tests do, so feel free to merge as is to prioritize having the coverage on the main branch.
Having said that, I believe my suggestions improve the overall test quality and maintainability, in particularly removing the async expectation and custom wait in favor of vanilla async await.
| @@ -24,7 +24,7 @@ final class NoteListMapperTests: XCTestCase { | |||
| /// Verifies that all of the Sample Notifications are properly parsed. | |||
| /// | |||
| func test_sample_notifications_are_properly_decoded() { | |||
| XCTAssertEqual(sampleNotes.count, 44) | |||
| XCTAssertEqual(sampleNotes.count, 46) | |||
There was a problem hiding this comment.
Probably out of scope, but have you considered extracting this given it's repeated in various places (4 times)?
There is an argument to be made for using literal values in tests assertions: It makes the assertion clearer, enhancing local reasoning.
On the other hand, replacing magic numbers with explanative constants can help readability and maintainability. E.g.
static let notificationLoadAllJSONCount = 46
Anyway, just an idea...
There was a problem hiding this comment.
Out of scope! – 🤔 I found it curious that the notification count had to change in two different test targets (NetworkingTests and YosemiteTests). I went to look at that JSON and found that it lives in NetworkingTests on the file system, but is in the YosemiteTests in the project structure
I did a text search for the file name and saw it's loaded via Loader in three different test targets
Loader is part of Networking
But, as far as I can tell, it's only used in the tests:
Addressing this (admittedly minor) inconsistency could be the opportunity to extract a test utils framework. It could be up to the utils framework to manage the JSON fixtures and return the notifications object from them. The expectation of the count being 46 could also be centralized there (unless I am mistaken in my understanding of why the assertions exists)
| /// Mock Dispatcher! | ||
| /// | ||
| private var dispatcher: Dispatcher! |
There was a problem hiding this comment.
Formatting nitpick:
Using /// makes Xcode parse the comment as documentation and add it to Quick help and generated headers.
/// comment
// comment
Given these are private vars, using the documentation comment (///) seems unnecessary.
| /// Mock stores manager | ||
| /// | ||
| private var storesManager: MockStoresManager! | ||
|
|
||
| /// Convenience Property: Returns the StorageType associated with the main thread. | ||
| /// | ||
| private var viewStorage: StorageType { | ||
| return storageManager.viewStorage | ||
| } |
There was a problem hiding this comment.
Thank you for documenting the rationale for having this convenience property.
As someone with not much context on the test suite, I don't see the connection between the storageManger and the main thread. How does it work?
| for note in sampleNotes { | ||
| if note.read == read, note.meta.identifier(forKey: .order) != nil { | ||
| return note | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Have you considered using a first(where:) here? It might make the code neater (via less indentation and offloading the return note / return nil to the stdlib.
| // Need to nuke this in-between tests otherwise some will randomly fail | ||
| NotificationStore.resetSharedDerivedStorage() |
There was a problem hiding this comment.
Could this run in the tearDown step, or is it best to keep it here because of state leaking from other tests?
As an aside, the fact that this is necessary points to some implicit state changes that we might want to look into. Unit tests are good at surfacing these possible issues.
There was a problem hiding this comment.
I copy/pasted it from other tests but now by looking at it more it looks like it can also be done in the tearDown, maybe even better would be to do it in both, to make sure to have clean storage before and after tests
| } | ||
|
|
||
| private func setupStoreManagerReceivingNotificationActions(for note: Yosemite.Note, noteStore: NotificationStore) { | ||
| self.storesManager.whenReceivingAction(ofType: NotificationAction.self) { action in |
There was a problem hiding this comment.
Nitpick: Is it necessary to access storesManager via self explicitly?
Would this work?
| self.storesManager.whenReceivingAction(ofType: NotificationAction.self) { action in | |
| storesManager.whenReceivingAction(ofType: NotificationAction.self) { action in |
The rationale behind this pedantic nitpick is that less code is usually better. Less for the reader to scan.
| if let note = sampleNote(read: false), let orderID = note.meta.identifier(forKey: .order) { | ||
| setupStoreManagerReceivingNotificationActions(for: note, noteStore: noteStore) | ||
| noteStore.updateLocalNotes(with: [note]) { | ||
| XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 1) | ||
| Task { | ||
| let result = await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(stores: self.storesManager, noteID: note.noteID, orderID: orderID) | ||
| switch result { | ||
| case .success(let markedNote): | ||
| XCTAssertEqual(note.noteID, markedNote.noteID) | ||
| let storageNote = self.viewStorage.loadNotification(noteID: markedNote.noteID) | ||
| XCTAssertEqual(storageNote?.read, true) | ||
| expectation.fulfill() | ||
| case .failure(let error): | ||
| XCTFail(error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| XCTFail() | ||
| } |
There was a problem hiding this comment.
Have you considered using XCTUnwrap to 1) Reduce indentation, 2) get a clearer error on which of the two steps failed?
let note = try XCTUnwrap(sampleNote(read: false))
let orderID = try XCTUnwrap(note.meta.identifier(forKey: .order))
setupStoreManagerReceivingNotificationActions(for: note, noteStore: noteStore)
noteStore.updateLocalNotes(with: [note]) {
XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 1)
Task {
let result = await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(stores: self.storesManager, noteID: note.noteID, orderID: orderID)
switch result {
case .success(let markedNote):
XCTAssertEqual(note.noteID, markedNote.noteID)
let storageNote = self.viewStorage.loadNotification(noteID: markedNote.noteID)
XCTAssertEqual(storageNote?.read, true)
expectation.fulfill()
case .failure(let error):
XCTFail(error.localizedDescription)
}
}
}|
|
||
| @MainActor | ||
| func test_markOrderNoteAsReadIfNeeded_with_network_unreadNote() async { | ||
| let expectation = self.expectation(description: "Mark order as read with network") |
There was a problem hiding this comment.
Picking up from @rachelmcr 's observation, I believe it should be possible to do away entirely with custom code do manage the async code.
@MainActor
func test_markOrderNoteAsReadIfNeeded_with_network_unreadNote() async throws {
let note = try XCTUnwrap(sampleNote(read: false))
let orderID = try XCTUnwrap(note.meta.identifier(forKey: .order))
network.simulateResponse(requestUrlSuffix: "notifications", filename: "notifications-load-all")
network.simulateResponse(requestUrlSuffix: "notifications/read", filename: "generic_success")
let result = await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(network: network, noteID: note.noteID, orderID: orderID)
switch result {
case .success(let markedNoteID):
XCTAssertEqual(note.noteID, markedNoteID)
case .failure(let error):
XCTFail(error.localizedDescription)
}
}This code seems better to me because:
- Uses no custom code, just Swift's
async / awaitand XCTest assertions - Has a one level indentation
- By removing closures, also removes the cumbersome
self.accesses
_Works on my machine_™
|
tnx @rachelmcr and @mokagio for the feedback! |
| } | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: Constants.expectationTimeout) |
There was a problem hiding this comment.
@bozidarsevo I haven't tried it on my local, but think the async await patter could be applied to this test, too?
There was a problem hiding this comment.
Problem is that updateLocalNotes function which uses completion block so I need to wait for it the be called to continue.
There was a problem hiding this comment.
One option here could be to use a continuation in the completion block. Something like this:
await withCheckedContinuation { continuation in
noteStore.updateLocalNotes(with: [unreadNote]) {
XCTAssertEqual(self.viewStorage.countObjects(ofType: Storage.Note.self), 1)
continuation.resume()
}
}
let result = await MarkOrderAsReadUseCase.markOrderNoteAsReadIfNeeded(stores: self.storesManager, noteID: unreadNote.noteID, orderID: orderID)
switch result {
case .success(let markedNote):
XCTAssertEqual(unreadNote.noteID, markedNote.noteID)
let storageNote = self.viewStorage.loadNotification(noteID: markedNote.noteID)
XCTAssertEqual(storageNote?.read, true)
case .failure(let error):
XCTFail(error.localizedDescription)
}There was a problem hiding this comment.
Totally forgot about that and I used it in the function itself already 🙈
Will update it! 👍
| } | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: Constants.expectationTimeout) |
There was a problem hiding this comment.
@bozidarsevo I haven't tried it on my local, but think the async await patter could be applied to this test, too?
Closes: #13851
Description
As a continuation on #13734 this PR adds tests for
MarkOrderAsReadUseCaseinMarkOrderAsReadUseCaseTests.We have 2 tests:
test_markOrderNoteAsReadIfNeeded_with_storeswhich testsstatic func markOrderNoteAsReadIfNeeded(stores: StoresManager, noteID: Int64, orderID: Int) async -> Result<Note, Error>test_markOrderNoteAsReadIfNeeded_with_networkwhich testsstatic func markOrderNoteAsReadIfNeeded(network: Network, noteID: Int64, orderID: Int) async -> Result<Int64, Error>Since both those functions have multiple actions or network calls inside, some mocks had to be updated to be able to test it.
notifications-load-all.jsonfile was also updated to have notifications used for these tests.Testing information
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: