DISC-369: Refactor ProjectPageViewModelTests (part one)#2913
DISC-369: Refactor ProjectPageViewModelTests (part one)#2913amy-at-kickstarter wants to merge 3 commits into
Conversation
…ile, ProjectPageViewModel_TrackingTests
amy-at-kickstarter
changed the base branch from
main
to
feat/adyer/split-project-vm-tests
| .observe(self.updateWatchProjectWithPrelaunchProjectState.observer) | ||
| } | ||
|
|
||
| func testConfigureChildViewControllersWithProject_WithFriendsNoBacking_ConfiguredWithProject() { |
There was a problem hiding this comment.
There are three tests for this VM output, but they're mostly testing what happens if the project friends fetch succeeds or fails. I removed that fetch ages ago, so I consolidated these down into a single test.
|
|
||
| func testConfigureChildViewControllersWithProject_FailedProjectFriendsNoBacking_ConfiguredWithProject() { | ||
| func testConfigureChildViewControllersWithProject_ConfiguredWithProject() { | ||
| let stubProject = self.projectWithEmptyProperties |
There was a problem hiding this comment.
In a lot of these tests, we aren't differentiating between the Project object that is passed in initially to the ProjectPageViewModel, and the Project object that is fetched a few seconds later. In many of these tests, I renamed them stubProject and project or stubProject and fullProject, for clarity.
| self.vm.inputs.configureWith(projectOrParam: .left(project), refInfo: RefInfo(refTag)) | ||
| self.vm.inputs.viewDidLoad() | ||
| self.vm.inputs.viewDidAppear(animated: false) | ||
| ProjectPageViewModelTests.mockNetworkRequests(project: project) { |
There was a problem hiding this comment.
This helper makes it much simpler to mock out all the network requests required. Many of these tests were mocking the wrong queries; mocking the queries incorrectly; or mocking no queries at all.
| self.vm.inputs | ||
| .configureWith(projectOrParam: .left(self.projectWithEmptyProperties), refInfo: RefInfo(.category)) | ||
| func testConfigureProjectPageViewControllerDataSource_loadsInitialProject_andRefreshedProject() { | ||
| ProjectPageViewModelTests.mockNetworkRequests { |
There was a problem hiding this comment.
Many of our tests in here weren't mocking network requests. These tests only pass because ProjectPageViewModel outputs the initial project that was added in configureWith(projectOrParam:). However, if this vm was initialized in the GraphQL way - using configureWith(projectOrParam: someParam) - the test would fail, because we aren't mocking and waiting for the actual network refresh.
By adding these mocks, I'll be able to clean up that inconsistent loading behavior, without causing all the tests to start failing.
amy-at-kickstarter
requested review from
a team and
stevestreza-ksr
and removed request for
a team
stevestreza-ksr
left a comment
stevestreza-ksr
left a comment
There was a problem hiding this comment.
Was looking at this, looks like you changed the branch base and that made the PR like 3x bigger, can you confirm this is expected?
2 participants
📲 What
Refactor
ProjectPageViewModelTests:configureInitialStatetoconfigureAndLoadmockNetworkRequests, which makes it easier to stub the 2-4 fetches required for this page🤔 Why
I'm working on some changes to the Project page loading for an A/B test. I realized pretty quickly that I need to tackle some of the refactoring for DISC-369 that I put off.
The refactoring was easy - but what actually turns out to be hard is fixing the tests once you do that refactor. This PR adds some utilities to make
ProjectPageViewModelTestsmore managable.Because this file is so long, I updated about half of the tests in this PR. I'll put up a second PR for the second half of the tests.