Consider narrowing the scope of GitExtensionsTest

[qmfrederik]

Many unit tests in the GitExtensionsTest class are covering core functionality of NerdBank.GitVersioning, mainly via the .GetHeight(...) extension methods.

I believe the result of the version height calculation should be considered observable behavior of the VersionOracle class, and most of the unit tests in the GitExtensions class could be moved to the VersionOracleTests class and invoke the VersionOracle class directly.

For example, there's GitExtensionsTest.GetVersionHeight_ProgressAndReset and VersionOracleTests.VersionHeightResetsWithVersionSpecChanges. I think they are both testing essentially the same observable behavior, and these unit tests should be located in the same class.

This would help unit testing the changes in #521 .

@AArnott what do you think?

[AArnott]

I tend to favor testing at the lowest level possible where public API makes sense. Now, maybe a lot of these GitExtensions shouldn't actually be public, but since they are, I would tend to exhaustively test them at that level, and then just have smoke tests at the higher level such as VersionOracle tests.
It sounds like you're saying we are testing the same thing (at the same complexity level) at both places though, and that would sound unnecessarily redundant. However I'd want high confidence that it was truly redundant before I would delete or trim a test down. The coverage we have is important to maintain.

[qmfrederik]

You raise a good point, though I think GitExtensions should not be public API (because it is closely coupled with LibGit2), and most of the methods in GitExtensions serve as helper methods which are used exclusively by unit tests.

Here's why. (I'll focus on the GetVersionHeight and GetHeight method for now, though most of the concerns apply to the other methods, too)

• Looking at the call sites for these methods (see below), only one 20-line overload is actually used by product code, by the VersionOracle class.
• All other overloads are only ever invoked by test code. In essence, they are shortcuts to enable testing of GitWalkTracker.
• If we're to make the switch to a managed Git implementation, GitExtensions can't really be much of a public API, since all of its methods are tightly coupled to the LibGit2 API.

To make sure we can re-use the unit tests which excercise GitExtensions.GetVersionHeight when we switch to a managed backend, I see two options:

• Rewrite the tests as unit tests create a new VersionOracle and validate the VersionHeight property.
• Refactor GitExtension.GetVersionHeight to not take Repository, Commit, Branch... objects as their arguments, but paths to Git repositories, SHA hashes of commits, branch names,... instead.

---

Here's the detailed overview of the various overloads and their call sites:

• int GetVersionHeight(this Commit commit, string repoRelativeProjectDirectory = null, Version baseVersion = null) is a 20-line method which is called
  • 6 times from the other GetVersionHeight/GetVersion overloads (see below)
  • Once from VersionOracle. I think it makes sense to move that method to VersionOracle, as a private method.
• int GetVersionHeight(this Repository repo, string repoRelativeProjectDirectory = null) is called exclusively by unit test methods. It's perhaps best to have it as a convenience method in the test project, if needed.
• int GetVersionHeight(this Branch branch, string repoRelativeProjectDirectory = null) is called twice by other overloads, and otherwise exclusively by unit tests
• int GetHeight(this Commit commit, Func<Commit, bool> continueStepping = null) appears to be dead code
• int GetHeight(this Commit commit, string repoRelativeProjectDirectory, Func<Commit, bool> continueStepping = null) is called exclusively by other overloads
• int GetHeight(this Branch branch, Func<Commit, bool> continueStepping = null) is called exclusively by unit tests
• int GetHeight(this Branch branch, string repoRelativeProjectDirectory, Func<Commit, bool> continueStepping = null) is called excusively by other overloads

[AArnott]

Great points. Hiding it all behind VersionOracle makes sense I guess, if the existing tests can be adjusted to keep testing what they were testing, but through the new front-end API. And for sanity in reviewing a PR diff, I'd very much prefer that we update the tests in place (i.e. not also move them to a new file) first, as an independent PR, and then as a second PR just move the tests to the VersionOracleTests.cs file. Or if one PR, as two commits so they can be diff'd independently.

Do you want to take this on? Your analysis and suggested course sounds good.
Just please be very careful about perf. We have caching built into multiple layers. Some of it very recently added, because it made a huge difference in build time. But I think you're on those threads, so I'm probably preaching to the choir here. :)