Skip to content

use "which" to locate git installation#999

Merged
wilbaker merged 2 commits into
microsoft:masterfrom
github:mac-git-installation-where
Apr 10, 2019
Merged

use "which" to locate git installation#999
wilbaker merged 2 commits into
microsoft:masterfrom
github:mac-git-installation-where

Conversation

@kivikakk
Copy link
Copy Markdown
Contributor

@kivikakk kivikakk commented Apr 3, 2019

Address this TODO -- the same code will work for Linux later. I added a test to exercise this -- it should pass on any VFSForGit running system as git should always be installed, but I have a feeling the unit test will run on Windows too and so will fail. How should I restrict a unit test to Mac(+Linux) only?

/cc @chrisd8088
/cc github#10

@kivikakk kivikakk changed the title use "where" to locate git installation use "which" to locate git installation Apr 3, 2019
@chrisd8088
Copy link
Copy Markdown
Contributor

Sweet! 👍

[TestCase]
public void GetInstalledGitBinPathWorks()
{
string gitBinPath = new MacGitInstallation().GetInstalledGitBinPath();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test to exercise this -- it should pass on any VFSForGit running system as git should always be installed, but I have a feeling the unit test will run on Windows too and so will fail. How should I restrict a unit test to Mac(+Linux) only?

VFS4G unit tests should not require that Git be installed and they should be able to run on all platforms (with the exception of the Windows unit tests that require .NET Framework).

If we really wanted to unit test the Mac's GetInstalledGitBinPath implementation we'd need to mock out the call to which. However, GetInstalledGitBinPath is hit quite frequently when cloning/mounting and so I think we're getting sufficient coverage of the method from the functional tests.

Copy link
Copy Markdown
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests should not require that Git be installed and should run on all platform (except when they require .NET Framework).

@chrisd8088
Copy link
Copy Markdown
Contributor

For my WIP branch, I confess I just removed the unit tests but kept the code change in 215357a, because it was really useful to not have the hard-coded reliance on /usr/local/bin/git on my dev system. :-)

@wilbaker
Copy link
Copy Markdown
Member

wilbaker commented Apr 9, 2019

For my WIP branch, I confess I just removed the unit tests but kept the code change in 215357a, because it was really useful to not have the hard-coded reliance on /usr/local/bin/git on my dev system. :-)

To make sure my earlier comments are clear, I think if the unit test is removed from this PR then it's good to go :)

@kivikakk
Copy link
Copy Markdown
Contributor Author

To make sure my earlier comments are clear, I think if the unit test is removed from this PR then it's good to go :)

Thanks for this! The unit test has been removed.

We presumably want some kind of exercise of the platform-specific functionality somewhere, though, I'd imagine. Where should those (eventually) go?

@kivikakk kivikakk force-pushed the mac-git-installation-where branch from 52e9800 to 791459c Compare April 10, 2019 05:26
kivikakk referenced this pull request Apr 10, 2019
Match the changes for macOS from 112ce00
for Linux, but exclude the new unit test as it currently fails on Windows.
@chrisd8088 chrisd8088 self-requested a review April 10, 2019 07:17
@wilbaker
Copy link
Copy Markdown
Member

We presumably want some kind of exercise of the platform-specific functionality somewhere, though, I'd imagine. Where should those (eventually) go?

Currently the platform specific code (like GetInstalledGitBinPath) is exercised by the functional tests.

@chrisd8088
Copy link
Copy Markdown
Contributor

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@wilbaker wilbaker merged commit 5618369 into microsoft:master Apr 10, 2019
@chrisd8088 chrisd8088 deleted the mac-git-installation-where branch April 10, 2019 17:55
@kivikakk
Copy link
Copy Markdown
Contributor Author

@wilbaker Thanks for the review and clarification there!

@jrbriggs jrbriggs added this to the M151 milestone Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants