Fix/tests (#1038)

* fix(refresh): update HomeViewController, HomeViewModel, prevent refreshing if a refresh is already occurring * fix(tests): update HomeViewModelTests (wip) * fix typo in logs * fix(tests): fix HomeViewModelTests * fix(tests): fix HomeViewModelTests
Pocket · Jul 15, 2024 · 423d6c2 · 423d6c2
1 parent ad8f748
commit 423d6c2
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 26 deletions.
diff --git a/PocketKit/Sources/PocketKit/Home/HomeViewController.swift b/PocketKit/Sources/PocketKit/Home/HomeViewController.swift
@@ -113,7 +113,13 @@ class HomeViewController: UIViewController {
         collectionView.delegate = self
 
         let action = UIAction { [weak self] _ in
-            self?.handleRefresh(isForced: true)
+            self?.handleRefresh(isForced: true) { [weak self] in
+                DispatchQueue.main.async {
+                    if self?.collectionView.refreshControl?.isRefreshing == true {
+                        self?.collectionView.refreshControl?.endRefreshing()
+                    }
+                }
+            }
         }
 
         collectionView.refreshControl = UIRefreshControl(frame: .zero, primaryAction: action)
@@ -165,12 +171,7 @@ class HomeViewController: UIViewController {
             overscrollView.heightAnchor.constraint(equalToConstant: 96)
         ])
 
-        model.fetch()
-        handleRefresh()
-    }
-
-    private func handleRefresh(isForced: Bool = false) {
-        model.refresh(isForced: isForced) { [weak self] in
+        handleRefresh { [weak self] in
             DispatchQueue.main.async {
                 if self?.collectionView.refreshControl?.isRefreshing == true {
                     self?.collectionView.refreshControl?.endRefreshing()
@@ -179,6 +180,10 @@ class HomeViewController: UIViewController {
         }
     }
 
+    private func handleRefresh(isForced: Bool = false, completion: @escaping () -> Void) {
+        model.refresh(isForced: isForced, completion)
+    }
+
     func handleBackgroundRefresh(task: BGTask) {
         model.refresh {
             task.setTaskCompleted(success: true)
@@ -187,7 +192,7 @@ class HomeViewController: UIViewController {
 
     override func viewWillAppear(_ animated: Bool) {
         super.viewWillAppear(true)
-        model.refresh { }
+        handleRefresh {}
     }
 
     required init?(coder: NSCoder) {

diff --git a/PocketKit/Sources/PocketKit/Home/HomeViewModel.swift b/PocketKit/Sources/PocketKit/Home/HomeViewModel.swift
@@ -43,6 +43,11 @@ enum ReadableSource {
     case external
 }
 
+enum RefreshState {
+    case loading
+    case ready
+}
+
 enum SeeAll {
     case saves
     case slate(SlateDetailViewModel)
@@ -144,6 +149,8 @@ class HomeViewModel: NSObject {
     private let sharedWithYouController: RichFetchedResultsController<SharedWithYouItem>
     private(set) var numberOfSharedWithYouItems = 0
 
+    private var refreshState: RefreshState = .ready
+
     init(
         source: Source,
         tracker: Tracker,
@@ -188,7 +195,6 @@ class HomeViewModel: NSObject {
                 self?.refresh(isForced: false) { }
             }
         }
-        fetch()
     }
 
     var isOffline: Bool {
@@ -200,7 +206,10 @@ class HomeViewModel: NSObject {
         // NOTE: despite HomeViewModel runs on MainActor, this call ends up on a different thread
         // when the app is backgrounded, thus we force it back to the main queue to avoid crashes
         // since these fetched result controller are created on viewContext
-        DispatchQueue.main.async { [unowned self] in
+        DispatchQueue.main.async { [weak self] in
+            guard let self else {
+                return
+            }
             do {
                 try recentSavesController.performFetch()
                 try recomendationsController.performFetch()
@@ -216,15 +225,21 @@ class HomeViewModel: NSObject {
     ///   - isForced: Whether or not the user forced the refresh
     ///   - completion: Completion block to call
     func refresh(isForced: Bool = false, _ completion: @escaping () -> Void) {
+        guard case .ready = refreshState else {
+            return
+        }
+        refreshState = .loading
         fetch()
 
         guard !isOffline else {
             completion()
+            refreshState = .ready
             return
         }
 
-        homeRefreshCoordinator.refresh(isForced: isForced) {
+        homeRefreshCoordinator.refresh(isForced: isForced) { [weak self] in
             completion()
+            self?.refreshState = .ready
         }
     }
 }

diff --git a/PocketKit/Sources/PocketKit/Refresh/FeatureFlagsRefreshCoordinator.swift b/PocketKit/Sources/PocketKit/Refresh/FeatureFlagsRefreshCoordinator.swift
@@ -58,7 +58,7 @@ class FeatureFlagsRefreshCoordinator: RefreshCoordinator {
             Log.debug("Already refreshing feature flags, not going to add to the queue")
             completion()
         } else {
-            Log.debug("Not refreshing  feature flags, to early to ask for new data")
+            Log.debug("Not refreshing  feature flags, too early to ask for new data")
             completion()
         }
     }

diff --git a/PocketKit/Sources/PocketKit/Refresh/HomeRefreshCoordinator.swift b/PocketKit/Sources/PocketKit/Refresh/HomeRefreshCoordinator.swift
@@ -65,7 +65,7 @@ class HomeRefreshCoordinator: RefreshCoordinator {
             Log.debug("Already refreshing Home, not going to add to the queue")
             completion()
         } else {
-            Log.debug("Not refreshing Home, to early to ask for new data")
+            Log.debug("Not refreshing Home, too early to ask for new data")
             completion()
         }
     }

diff --git a/PocketKit/Sources/PocketKit/Refresh/TagsRefreshCoordinator.swift b/PocketKit/Sources/PocketKit/Refresh/TagsRefreshCoordinator.swift
@@ -58,7 +58,7 @@ class TagsRefreshCoordinator: RefreshCoordinator {
             Log.debug("Already refreshing Tags")
             completion()
         } else {
-            Log.debug("Not refreshing Tags, to early to ask for new data")
+            Log.debug("Not refreshing Tags, too early to ask for new data")
             completion()
         }
     }

diff --git a/PocketKit/Tests/PocketKitTests/HomeViewModelTests.swift b/PocketKit/Tests/PocketKitTests/HomeViewModelTests.swift
@@ -128,8 +128,7 @@ class HomeViewModelTests: XCTestCase {
         let viewModel = subject()
 
         let receivedLoadingSnapshot = expectation(description: "receivedLoadingSnapshot")
-        receivedLoadingSnapshot.assertForOverFulfill = false
-        viewModel.$snapshot.dropFirst(2).sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { receivedLoadingSnapshot.fulfill() }
             XCTAssertEqual(snapshot.sectionIdentifiers, [.loading])
         }.store(in: &subscriptions)
@@ -157,7 +156,7 @@ class HomeViewModelTests: XCTestCase {
 
         let viewModel = subject()
         let receivedSnapshot = expectation(description: "receivedSnapshot")
-        viewModel.$snapshot.dropFirst().first().sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { receivedSnapshot.fulfill() }
             XCTAssertEqual(
                 snapshot.sectionIdentifiers,
@@ -232,7 +231,7 @@ class HomeViewModelTests: XCTestCase {
 
         let viewModel = subject()
         let receivedEmptySnapshot = expectation(description: "receivedEmptySnapshot")
-        viewModel.$snapshot.dropFirst().first().sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { receivedEmptySnapshot.fulfill() }
 
             XCTAssertEqual(
@@ -327,7 +326,7 @@ class HomeViewModelTests: XCTestCase {
         viewModel.fetch()
 
         let snapshotExpectation = expectation(description: "expected snapshot to update")
-        viewModel.$snapshot.dropFirst(2).sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { snapshotExpectation.fulfill() }
 
             XCTAssertEqual(
@@ -372,7 +371,7 @@ class HomeViewModelTests: XCTestCase {
         viewModel.fetch()
 
         let snapshotExpectation = expectation(description: "expected snapshot to update")
-        viewModel.$snapshot.dropFirst().sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { snapshotExpectation.fulfill() }
 
             XCTAssertEqual(
@@ -418,7 +417,7 @@ class HomeViewModelTests: XCTestCase {
         viewModel.fetch()
 
         let snapshotExpectation = expectation(description: "expected snapshot to update")
-        viewModel.$snapshot.dropFirst().sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             defer { snapshotExpectation.fulfill() }
 
             XCTAssertEqual(
@@ -452,9 +451,10 @@ class HomeViewModelTests: XCTestCase {
         )
 
         let viewModel = subject()
+        viewModel.fetch()
 
         let snapshotExpectation = expectation(description: "expected snapshot to update")
-        viewModel.$snapshot.dropFirst().sink { snapshot in
+        viewModel.$snapshot.dropFirst(4).first().sink { snapshot in
             defer { snapshotExpectation.fulfill() }
 
             XCTAssertEqual(
@@ -495,9 +495,9 @@ class HomeViewModelTests: XCTestCase {
         networkPathMonitor.update(status: .unsatisfied)
 
         let snapshotExpectation = expectation(description: "expect a snapshot")
-        snapshotExpectation.assertForOverFulfill = false
         let viewModel = subject()
-        viewModel.$snapshot.dropFirst(2).sink { snapshot in
+
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             XCTAssertEqual(
                 snapshot.itemIdentifiers(inSection: .recentSaves),
                 [
@@ -521,8 +521,7 @@ class HomeViewModelTests: XCTestCase {
         let viewModel = subject()
 
         let snapshotExpectation = expectation(description: "expected a snapshot update")
-        snapshotExpectation.assertForOverFulfill = false
-        viewModel.$snapshot.dropFirst(2).sink { snapshot in
+        viewModel.$snapshot.dropFirst(2).first().sink { snapshot in
             XCTAssertNotNil(snapshot.indexOfSection(.offline))
             XCTAssertEqual(snapshot.itemIdentifiers(inSection: .offline), [.offline])
             snapshotExpectation.fulfill()