fix: use public 'table_admin_client' property in backups methods

[kolea2]

calling _table_admin_client was lazily evaluated and would return a NoneType in cases where table operations were not already done.

[kolea2]

@crwilcox any ideas on how to reliably test this?

[tseaver]

Because a Python property looks just like an attribute, there isn't any good way to distinguish them. You could add a property to the dummy _Client class which returned the value of a different attribute, e.g.:

class _Client(object):
    _different_tac = None

    def __init__(self, project=TestBackup.PROJECT_ID):
        self.project = project
        self.project_name = "projects/" + self.project

    @property
    def table_admin_client(self):
        return self._different_tac

and then populate _different_tac in the tests.

[tseaver]

I just checked, and there is a mock.PropertyMock which might do, but it is too gnarly to use IMHO.

[kolea2]

Because a Python property looks just like an attribute, there isn't any good way to distinguish them. You could add a property to the dummy _Client class which returned the value of a different attribute, e.g.:

class _Client(object):
    _different_tac = None

    def __init__(self, project=TestBackup.PROJECT_ID):
        self.project = project
        self.project_name = "projects/" + self.project

    @property
    def table_admin_client(self):
        return self._different_tac

and then populate _different_tac in the tests.

@tseaver discussed with @crwilcox, we decided to go with a slightly different approach here. In the backups system test, reset the Client so that we can assert the table admin client object is initialized and returned. I tested this with and without my patch to confirm this works. LMK what you think!

[tseaver]

@kolea2 I don't think I grok the need for the system test change. If you want to ensure that all the methods which use the table admin client use the property, rather than the attribute, then just setting the same-named attribute on the _Client shim in the unit tests should do the trick: any methods which try to use _table_admin_client will fail.

[crwilcox]

The system test change ensure we start with all lazy-loading reset.

As for testing, the issue is we mock that bit out as best I can tell. The changes @kolea2 made means the mock no longer even has the internal surface, so it def. isn't being called.

per @crwilcox's comment, will leave testing to system.py. We can revisit adding more mocked unit tests if we find it necessary in the future.