feat: Implement tests

[martinalbert]

No description provided.

[Toreno96]

I was able to finally review the unit tests themselves, and I have bad news: I think all of those require reimplementation.

Let's see an example, TestListAuthor 👇

We mock Author.all (the function on the highest possible level of abstraction) to return list_authors():

buttercms-python/butter_cms/tests/test_author.py

Lines 8 to 11 in e974175

	@patch('butter_cms.author.Author.all')
	def test_all(self, mock_all):
	mock_all.return_value = list_authors()
	author = Author('<demo-token>')

So we don't really test any internals of the Author class other than the initialization of the class, but even that is not much because the only thing the initialization does is assigning/defining a few member variables, which we then don't use at all because of the mock.

So:

    @patch('butter_cms.author.Author.all')
    def test_all(self, mock_all):
        mock_all.return_value = list_authors()
        author = Author('<demo-token>')
        response = author.all()
        …

This is effectively the same as:

    def test_all(self):
        response = list_authors()
        …

Furthermore, the whole test is effectively the same as:

    def test_all(self):
        response = {
            'data': [
                {
                    'first_name': 'John', 
                    'last_name': 'Doe', 
                    'slug': 'john-doe'
                },
                {
                    'first_name': 'Jane', 
                    'last_name': 'Doe', 
                    'slug': 'jane-doe'
                }
            ]
        }

        self.assertIsNotNone(response)
        self.assertIn('data', response)

        data = response['data']
        self.assertIsNotNone(data)
        self.assertIsInstance(data, list)
        self.assertGreater(len(data), 0)

        author_data = data[0]
        self.assertEqual('John', author_data.get('first_name'))
        self.assertIsNone(author_data.get('recent_posts'))

I think at this point you can see, that this is kind of the same as assert true == true; which is not a good unit test 😅

As such, I think all the current tests have to be removed and rewritten.

I was thinking what could be a good approach. We can't mock all() or get() or similar because of the above reasons.
I don't think we can mock Client.api_get() either because in a significant number of cases all() or get() is just passing through the result of api_get, and only in some cases it does a bit of additional work, so mocking api_get() would be the same as mocking all() or get() for the most part.

I believe a good approach could be to rely on https://pypi.org/project/responses/ to mock responses from request library (which we use inside the api_get) without mocking any method of Client and derivatives themselves. Something like (pseudocode):

    def test_all(self):
        # mock an appropriate response(s) using `responses`
        …
        # then
        author = Author('<demo-token>')
        response = author.all()
        …

This is not perfect either because unit tests aren't supposed to know that the tested function is using library X or Y internally, but I don't see any better solution, unfortunately.

@martinalbert WDYT?

[martinalbert]

@Toreno96 thank you for the review, I understand your concerns and I don't like this approach as well.

I tried the responses package as well as vcr and I'm thinking if it wouldn't make more sense to use the VCR while we're at it. I tested it on one test and you can see the results in this commit: 62f5980

[martinalbert]

@Toreno96 Just to let you know why I tried also the VCR along the responses

• They both solve the mentioned concern of superficial unit tests - intercepting underlying HTTP traffic instead of method calls,
• It feels like similar amount of work would be needed to make the tests better,
• VCR removes the need to create mocks manually and uses more realistic data

[Toreno96]

Looks much better now!

Please see:
#11 (comment)
#11 (comment)

Also, I see that in a few places you're defining variables like f = data['navigation_menu_item'], d1 = data[0], etc.

Could you rename them all to something more meaningful?
Single-letter variables are unclear and difficult to search, and it's not like we have a character limit or anything.

[Toreno96]

LGTM