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

On Tue, Apr 24, 2018 at 3:27 AM, Robie Basak <email address hidden>
wrote:

> 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.
>

Yes that makes sense. On some level, I wanted a programmatic way to check
that my changes did what I wanted and a unit test for it made sense, even
if it's fugly :) I am also happy to not land the test if it's too fragile
in your opinion, as I think it demonstrates the fix for the purpose of
review still.

> > > 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.
>

Ok -- I'm having trouble following the potential hypothetical, but I
believe you and understand your perspective.

>
> > 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.
>

Right, I meant before landing it now, but I see your point.

> > 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.
>

This seems reasonable and I can work on that today, I think.

> > 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).
>

Yep I have not been good about writing docstrings for the tests, I'm sorry.

> > > 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.
>

Of course!