Demo Unit test for CloudStorageAPIClient

[chelseatroy]

Goal(s) of this Pull Request:

Figure out unit testing. This might be a conversation starter as opposed to a final implementation.

This test never calls the google.cloud APIs directly; instead, it mocks out calls to that API and checks that those mocked calls were made.

As you can see from the commit history, I first rolled my own mocks manually.. Then in a follow-up commit I switched to this third party mock GCS client for Python.

This library tests against the real functions, I'm guessing, whenever the maintainers run the tests, so it seemed like the best choice I could find. This does mean our unit tests now depend on a third party library with six stars maintained by three people, none of whom work at Google, at the rate of maybe a commit per month, which does not suggest active monitoring or development.

To wit: the library doesn't support client.get_bucket(), only client.bucket(). So I had to change our implementation to be able to use this mocking library, even though our original implementation was functional. For our specific use case the switch didn't matter, but the two functions do not do exactly the same thing.

This could have introduced regression risk, for the sake of working with the mocking library, and is an example of the reason I have misgivings about mocking libraries from people other than the maintainers of the thing being mocked. We could make a PR against the library repository to add .get_bucket(), but if we're considering forking a library the very first time we use it, I think that raises valid questions about the suitability of the library for our use case.

I'll share why I opted for the library rather than manual shallow mocks later in this description. How did I check for a behavior regression while making the switch? I ran our integration tests.

The issues with the integration tests for this functionality are:

• they are slow
• they require google cloud authentication

For that reason, we turned them off by default and will run them on CI instead of having them run anytime developers try to do a test loop.

It would be nice to have a way to test this functionality that's fast, that doesn't require authentication. But the integration test has this important advantage:

• It tests the behavior, rather than the implementation. If our integration with google.cloud API is broken in such a way that our files are not getting stored or fetched, it fails. If Google changes the behavior of their API in a way that affects us, it fails. If we change our function calls in a way that produces a regression, it fails. We then get to find out from our test suite that our tool is broken, instead of from our consumers. That's valuable.

Meanwhile, as long as our functions succeed at storing and fetching, the integration tests succeed. They don't check how we make that happen beyond calling the functions themselves. They are how I knew within seconds, when I had to switch the implementation to satisfy the unit test mocking library, that things still worked.

By contrast, this unit test in this PR tests exclusively implementation, and not behavior.

The circumstances under which this test would fail would be:

• Someone comments out/removes a call to the google.cloud API and forgets to put it back. Because integration tests will run on CI and those tests will also fail if this happens, this mistake would not slip into the main branch anyway.
• The google API changes in such a way that a programmer has to change how we call it in order to restore the behavior we want (which we check in the integration tests), and they don't change these unit tests to match the new implementation changes. In this case, I'd argue, these tests are not catching the problem; they instead are just adding an extra step before a programmer can merge a fix for the problem. This feels, to me, like a suboptimal use of developers' time and attention.

Why I switched to the library from shallow mocks:

• Google's API is heavily nested: call a function to get a Google object, call that object to get another google object, then call that object. So we have to mock three objects here, which feels especially brittle. The mocking library's functionality checks against the behavior of GCS a little more than three shallow mocks did.
• In order to check the use of three shallow mocks, we needed a way to access their return values from the test. The options are to:

1. Always return them, which would be very confusing for consumers who don't need any of these objects, or
2. Do what I did and add a testing flag to the function arguments that changes the return type of the function. Although this is better for our consumers, it's worse for maintainers and contributors: they now have to figure out why the functions' return type changes. Having the return type of a function change is an unconventional choice and one that adds overhead to understanding this API client's behavior. The library permits me to not have to choose between these two evils.

However, no matter how we unit test this, to me it feels brittle. If these tests fail, we have to run the integration tests to verify whether anything is actually wrong, and if so, what precisely is wrong. Is it worthwhile to have these two tests that operate by a mocked API proxy, such that they run quickly but might fail for the wrong reasons? Maybe. But it's worth explicitly calling out, so I added comments to that effect.

Acceptance criteria:

• pytest shows 2 tests passing
• pytest -m integration shows 2 tests passing
• Commenting out any call made against blob in cloud_storage_api_client.py causes a unit test and an integration test to fail

[chelseatroy]

force-pushed to do an interactive rebase of two commits I'd already pushed that were really part of the same change set.