fix(778): git bug rm broken

[smoyer64]

Both the caching layer and the underlying repository were performing a mutex lock while resolving the bug prefix. This resulted in the outer lock succeeding and the inner lock being permanently blocked. Since the inner locking was guarding the vulnerable code, removing the outer lock fixes the problem without causing concurrency issues.

Resolves #778

[smoyer64]

@MichaelMure - This PR is a bit bigger than I'd like but the non-test code changes are pretty small. I'll annotate them with comments as I want to make sure these don't break anything. The rest of the code tests the following Cobra commands:

• root (show top-level help)
• user create
• add bug
• rm bug

[vincetiu8]

LGTM!

[MichaelMure]

A pretty cool thing to have, just some comments for polishing

[smoyer64]

@MichaelMure - I actually wanted to talk about the use of "golden files" (as described by Mitchell Hashimoto's "Advanced Testing with Go") in git-bug tests. Using this technique may not be worthwhile as I'm not sure that it's applicable to any of the operations except the various help output. Other commands commonly return output that includes hash prefixes, so they won't be consistent.

The benefit of this type of test is that it will flag any time the help output changes and presumably the developer would either fix the code that caused the change or run the test with the -update flag so that the new output was the expected value. The downside is that, if the text output by help commands changed frequently, it might feel onerous to repeatedly run the update command.

The test failure on Windows is due to the differences in line termination characters - I'll fix this (and address printing the output as strings) to make sure the tests actually complete successfully, then will move on to addressing your other comments. At some point, we'll also need to redirect stdin (for testing commands that can be interactive) and there are a few places that produce output without using env.err or env.out.

Finally, I was wondering why the user_create command outputs to both env.err and env.out on success -

git-bug/commands/user_create.go

Line 93 in dd8134b

env.err.Println()

.

[MichaelMure]

Yeah, the benefit of those golden files is not obvious to me when in balance with the complexity they bring and what can be tested with it. It seems to me that most of the time it would be testing Cobra or the few data formatting functions.

It might be better to assert the result of the tests by simply checking things on the backend rather than the actual output. Data formatting functions (like lsJsonFormatter) can be isolated and tested individually with no backend.

Then comes the problem of the interactive commands. I suppose the same approach combined with stdin manipulation would work best?

Finally, I was wondering why the user_create command outputs to both env.err and env.out on success -

I'm not sure but I think the idea was to have a proper new line in the terminal, yet have the output clean when reading stdout with a pipe or a program. That might not make sense.

[MichaelMure]

To be clear, it might be that golden files make sense in some case, it's just that testing with simpler method could go a long way before that's needed. Not to prevent you from doing it though :-)

[smoyer64]

I'm not sure but I think the idea was to have a proper new line in the terminal, yet have the output clean when reading stdout with a pipe or a program. That might not make sense.

That makes perfect sense (and follows the "Unix way") - I'll have to watch for that in other commands as I expect to continue writing tests for the commands package.

With respect to using golden files, after fixing the Windows build, I'll mark that test with t.Skip() until there's a clear use case. For the add, rm and user_create commands, I'm using other assertions anyway

[smoyer64]

@MichaelMure - I've redone the commands testing to exclude the creation of the Cobra commands. Essentially there's now a testEnv that embeds the Env needed for testing but also the cwd and redirected out needed to make assertions. I usually prefer to test via exported methods from a separate test package but this code is now more consistent with the tests in other packages.

Next steps - submit a PR that moves all creation of GoGitRepo used by tests to a t.Helper() and then continue chasing down the Windows concurrency issue when tests include t.Parallel(). I've now got a Windows laptop so the test loop should be way shorter!

[MichaelMure]

Awesome, thanks!

[MichaelMure]

Feel free to merge when that make sense to you.