CB-369: Write a review page shows incorrect information depending on if the item is a work or artist

[amCap1712]

Use both entity type and mbid as cache key

JIRA: CB-369

[alastair]

Did you take a look at the gen_key documentation? https://github.com/metabrainz/brainzutils-python/blob/77fa8df2cf5f51bf7886799449da3a5eef51eccd/brainzutils/cache.py#L250
I understand that it's designed so that you can pass in as many arguments as you want and they will be concatenated.
In this case, we could do something like

 key = cache.gen_key('artist', mbid)

so that the key will end up as something like artist_c0b2500e-0cef-4130-869d-732b23ed9df5

Can you confirm what the behaviour is with this fix if we visit /review/write/work/[artistmbid]? Will it correctly raise an error? Can you double-check that there is a test for this case?

[amCap1712]

Did you take a look at the gen_key documentation? https://github.com/metabrainz/brainzutils-python/blob/77fa8df2cf5f51bf7886799449da3a5eef51eccd/brainzutils/cache.py#L250
I understand that it's designed so that you can pass in as many arguments as you want and they will be concatenated.
In this case, we could do something like

 key = cache.gen_key('artist', mbid)

so that the key will end up as something like artist_c0b2500e-0cef-4130-869d-732b23ed9df5

No, I actually forgot to look at the documentation for gen_key. I'll fix this.

Can you confirm what the behaviour is with this fix if we visit /review/write/work/[artistmbid]? Will it correctly raise an error?

Yes, I double checked that there is an error displayed for the case.

Can you double-check that there is a test for this case?

I don't think there is a test case for this or even if there were one this error would probably slip past it because of mocking the cache in tests. But now that I look again I was unable to find any instance of mocking the cache except test_statistics.py. I am still unsure what would be a good test case for this. Let's discuss this on IRC and then I'll add the tests for this.

[amCap1712]

I have updated the tests. However, the tests are failing if run together. If only the cache tests are run, they pass successfully. I think I have narrowed down the problem. It seems that it is an issue with how other tests mock the methods we are trying to test. But I am not sure.

[amCap1712]

I have fixed the issue and pushed the changes. The statistics_test was mocking cache.gen_key which was interfering with the cache tests. For now, I have removed that mock but there are multiple other instances of such mocks that will interfere with other tests. We should later work on reducing the scope of such mocks.

[alastair]

I guess we don't use @mock.patch here because otherwise we would have to put it on all of the tests.
However, this test is buggy. We can't override something in setUp without undoing it in tearDown.
I see two possible fixes to this:

def setUp():
   self.old_gen_key = cache.gen_key
   cache.gen_key = MagicMock
def tearDown():
  cache.gen_key = self.old_gen_key

alternatively, we just @mock.patch every test. I like this solution because it's a lot more explicit as to what is happening

[alastair]

@amCap1712 can you rebase after the flake8 changes?
Did we decide to use @mock.patch on all tests in statistics_test? We can open a ticket for other cases in other files where mocks are being used incorrectly, but I think it makes sense to fix statistics_test.py in this PR, as it came up as problematic when we made these changes.

[amCap1712]

I'll do the rebase and update the PR. Yeah, we decided to @mock.patch everywhere we found the issue of incorrect mocking. Sure, I'll fix the statistics_test.py in this PR.

[alastair]

Thanks, these changes look a lot better. Looking through the statistics_test file I see two other things:

• lots of print()s, these should be removed
• Do we actually test the cache mocks? what's the point of mocking them if we don't do an assert_called_with? Is it just to prevent it from trying to access a real reddis?

[amCap1712]

I think the only purpose here is to disable the redis cache as the comment said in the setUp mehtod but I may be wrong.

[alastair]

are there any tests that check if these methods correctly write to the cache when called?
This should be quite simple to add. Just cache_get.assert_called_once_with([args])

[amCap1712]

Sure, I'll add the checks.

[amCap1712]

I have added the check. I have also removed the mocks from other methods where only the db is being queried and cache is not involved.

[alastair]

I have also removed the mocks from other methods where only the db is being queried and cache is not involved.

That's great! This is an advantage to explicitly using mock.patch - we only patch things that we need to