Fix config lookup#581
Conversation
|
What are the symptoms of this bug? |
No mountId / enlistmentId in telemetry :-(. Very bad! |
|
Got it. I would vote to get the test fixes into this PR. There's no pending release, so this is not an emergency fix. |
| string value; | ||
| string error; | ||
| if (!configResult.TryParseAsString(out value, out error, defaultValue: string.Empty)) | ||
| if (configResult.TryParseAsString(out value, out error, defaultValue: string.Empty)) |
There was a problem hiding this comment.
Because the configResult relies on a Git process, this is hard to unit test. Is it possible to mock out GetGitProcess() to inject a custom GitProcess class that returns certain values for GetFromLocalConfig()?
There was a problem hiding this comment.
Yes that is the purpose of Enlistment.CreateGitProcess. It is already overridden by MockGVFSEnlistment to return a MockGitProcess.
I'm not sure when GVFSEnlistment.GetGitProcess was added, but it looks like we can probably drop it as a duplicate.
There was a problem hiding this comment.
Good point. I didn't see those mocks initially, so I recreated these things to get a proof of concept. @jeschu1 you could take those ideas and do them correctly by using the existing mocks.
There was a problem hiding this comment.
Thanks @sanoursa and @derrickstolee !
This was a good opportunity to clean the class up (dropped GetGitProcess) and add tests. The MockGVFSEnlistment works nicely here.
Let me know what you think.
jeschu1
force-pushed
the
fix
branch
2 times, most recently
from
a4bbb9a to
9cbe856
Compare
| [TestFixture] | ||
| public class GVFSEnlistmentTests | ||
| { | ||
| public const string MountId = "85576f54f9ab4388bcdc19b4f6c17696"; |
There was a problem hiding this comment.
We are very inconsistent here. We have public constants in MoveRenameFileTests but private constants in MoveRenameFileTests_2 and protected constants in GitRepoTests.
It does seem like we use private more often. This could be something to add to the contributor style guide (#537) and to consolidate into a cleanup PR during an expert week. (not in scope for this PR, except to change this variable.)
There was a problem hiding this comment.
They shouldn't be private, updated :-).
And yes @derrickstolee we can consider fixing scopes that are wrong across the board, my only concern with those types of changes is how it make the history look.
| [TestCase] | ||
| public void CanGetMountId() | ||
| { | ||
| MockGVFSEnlistment enlistment = this.CreateEnlistment(); |
There was a problem hiding this comment.
It's a little bit odd to use a mock object as the object under test. The more pure answer would be to update GVFSEnlistment to take a factory method as an argument, but that is overkill here, and would unnecessarily complicate the product code.
You might consider creating a private class here named TestGVFSEnlistment that derives from GVFSEnlistment and use that instead, so that future enhancements to the common mock object don't interfere with these unit tests.
There was a problem hiding this comment.
@sanoursa you're correct here. I created my own TestGVFSEnlistment, so we're protected against future changes to the MockGVFSEnlistment.
I saw [a broken unit test build](https://gvfs.visualstudio.com/ci/_build/results?buildId=4499) in #581, but it wasn't due to a change there. It was due to an incorrect handling of a `null` member in `GitMaintenanceQueue` and how it returned `true` if the blocking collection was `null`. If the timing is just right, that value wasn't `null` and the output was `false`. Fix this by doing the right thing: return `false` when the collection is `null`, and change the test to expect this.
4 participants
This is a bad bug caused by yours truly. I'll look at ways to unit/functional test, but in the interim let's fix it.