Code review comment for ~nacc/git-ubuntu:importer-cleanup-tmpdir-always

On Mon, Apr 23, 2018 at 07:51:19PM -0000, Nish Aravamudan wrote:
> I'm also not sure how to square 'not worth having a test' with ...
> everything new should have a test.

I read it as "everything new should have a test where it makes sense in
the code to do so". Pragmatically, there's no point in writing a test if
we don't see a way to write it such that the potential benefit outweighs
the cost. In this case I think this is quite close to the line. But
given that you've written it and it works, I'm thinking that there's no
harm done in landing it. Perhaps if it causes a future problem, I'll
propose to remove it then rather than fixing it.

> > What if the function under test (basically the entire program) creates temporary
> > files in some other way, or leaves junk somewhere else?
>
> That would be a code change in the importer, no? And if that was to
> happen, afaict, this test would presumably break.

I think in this case, depending on the specifics, the test is likely to
continue passing while not testing what it is supposed to be testing.
This is why I think it is of limited benefit for us to have it.

> I suppose we could change the git_repository code (refactor) to not
> invoke mkdtemp but a helper function to create the repository and mock
> that?

We could, but then we'd need to remember in the future that this is
required to make the test work. Refactoring it later without that in
mind will result to the same I-broke-it-but-the-test-still-passes
result.

> The real reason for this, and not just passing a directory in
> directly, is that in the current code passing a directory argument
> implies no_clean. So we wouldn't clenaup in that case.
>
> We could alternatively abstract out the "should I clean" decision
> (which would take the directory and no_clean cli values as arguments)
> and then mock that only. Then we would be able to pass in a directory
> argument and expect it to be removed in the error path regardless of
> what else the importer writes.

What if we give GitUbuntuRepository a close() method, move to it a
no_clean (or more clearly delete_on_close) attribute, and give the class
the responsibility of cleaning up?

We could then separately unit test that the class does clean up and does
not clean up as required based on the attribute, and that the main
function does correctly call the close() method on some specific causes
of failure.

> Note, that your point above about 'junk' is outside the scope of what
> I'm testing here. If we do create other junk, then that would (imo) be
> a different test. What I'm testing here is that the repository created
> by the importer is removed on failure/error.

Fair enough. I hadn't got that from
test_importer_main_cleanup_on_exception. We should make it clear then
that this is exactly the scope of the test (by name or docstring).

> > What if the function fails before it does anything due to the way it is
> > called from this test? Might be better to look for a more specifically
> > defined exception (which we raise from the mock) rather than doing
> > a catch-all on it.
>
> Yeah I can make this a RuntimeError.

I suggest something even more specific like:

    class MockError(RuntimeError): pass

...because otherwise a real RuntimeError would elide the test in the
same way as my objection to using Exception.