CB-388: Reduce scope of mocks #316
Conversation
amCap1712
force-pushed
the
CB-388
branch
6 times, most recently
from
ccb6203
to
a6e3a78
Compare
|
|
||
| # test when _fetch_access_token returns expired token | ||
| requests.get = MagicMock(return_value=self.unauth_response) | ||
| spotify._fetch_access_token = MagicMock(return_value="expired-token") | ||
| requests_get.return_value = self.unauth_response |
There was a problem hiding this comment.
if we use the mock twice in a single test, we should use .reset_mock() on it before setting new values and asserting that it is called
| @mock.patch('critiquebrainz.frontend.external.spotify.get_multiple_albums') | ||
| @mock.patch('critiquebrainz.frontend.external.mbspotify.add_mapping') | ||
| @mock.patch('critiquebrainz.frontend.external.spotify.get_album') | ||
| @mock.patch('critiquebrainz.frontend.external.spotify.search') |
There was a problem hiding this comment.
are all of these mocks used in each of the tests that are in this method? I think it might be more explicit if we have separate methods for each, and we only add mocks for the methods that we know we need to mock.
However, if you want to check assert_not_called on these mocks, perhaps this is ok.
There was a problem hiding this comment.
Yeah, I checked and removed some unneeded mocks after your review but these four are needed and and cannot be removed. We could break this into parts but at least one test requires all 4 mocks.
There was a problem hiding this comment.
Also, this is a part of the mapping code. I hope this will soon get removed once we add BrainzPlayer.
| self.temporary_login(self.reviewer) | ||
| get_release_group.return_value = self.test_entity |
There was a problem hiding this comment.
would be nice to assert_called_with this mock, and other mocks in similar tests
There was a problem hiding this comment.
Can you explain what you mean by similar tests ?
There was a problem hiding this comment.
Also, this was mock turned out to be unneeded so I removed it.
| # create a review by self.user and check it's not hidden | ||
| review = self.create_dummy_review() | ||
| self.assertEqual(review["is_hidden"], False) | ||
|
|
||
| # make self.hacker as current user | ||
| self.temporary_login(self.hacker) | ||
| is_user_admin.return_value = False |
There was a problem hiding this comment.
It's be better to test this by adding the username to the admin list in the config, instead of mocking this methods.
With CB-385 this will become easier.
There was a problem hiding this comment.
Sure, let's leave this the way it is for now. I'll add a note to CB-385 to fix this as a part of it.
amCap1712
force-pushed
the
CB-388
branch
3 times, most recently
from
dba0c4f
to
60a5970
Compare
|
@amCap1712 I rebased this against master to fix the failing tests that I fixed. I think that we should do it again after we merge #323 so that we can see if there are any new lint warnings introduced by this change. I think I see a few of them, but they're mixed up with the other ones that I fixed. It looks like the failing tests are new, and we should look at those. I'm happy to take over this PR if you're busy. Let me know what you think of #323 and I'll get on it. |
|
@brainzbot test this please |
|
This is working for me now, I had a bit of trouble with tests on jenkins - the timeout to the musicbrainz db is about 6 minutes, and it takes a bit longer than this for the sample database to load when the CI machine is under load. I'm not sure what the best solution to this is, it's annoying to have to download and import this data for every single test run, but I don't think there's an easier way to make this load faster, unless we a) mount postgres data directory into the host (causes problems if we run two tests at the same time, makes it harder to reset the data), or b) push a db image that already includes this data (would be large, and would need manual management to push new versions with new data) |
|
@brainzbot retest this please |
|
The CI is passing now. However, there is still an issue with the database not being up while running tests locally. Could it be due to some difference in what |
CB-388