-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use latest version of googletest (v.1.11.0) for testing across all platforms. #997
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment about moving googletest vcxproj files, but good to commit if wanted.
@@ -103,7 +103,7 @@ | |||
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'"> | |||
<ClCompile> | |||
<Optimization>Disabled</Optimization> | |||
<AdditionalIncludeDirectories>..\..\include;..\..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> | |||
<AdditionalIncludeDirectories>..\..\..\..\third_party\googletest\googlemock\include;..\..\..\..\third_party\googletest\googlemock\;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this file into the test folder? Maybe something like tests/googletest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These include files are coming from googletest submodule, which is configured in <repo>\third_party
folder. But we can move vsproj files from <repo>\googletest
to <repo>\test
. I kept it as it is, so as not to change their references in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move them. it would be weird to have random googletest/msvc 2015 files in a folder when it is only intended for tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Will move them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment around removing sub-folders where possible, which you're welcome to do as a follow-up change.
@@ -28,10 +28,10 @@ | |||
</ProjectConfiguration> | |||
</ItemGroup> | |||
<ItemGroup> | |||
<ProjectReference Include="..\..\googletest\googlemock\msvc\2015\gmock.vcxproj"> | |||
<ProjectReference Include="..\googletest\googlemock\msvc\2015\gmock.vcxproj"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also remove the msvc\2015
path as well?
<Project>{34681f0d-ce45-415d-b5f2-5c662dfe3bd5}</Project> | ||
</ProjectReference> | ||
<ProjectReference Include="..\..\googletest\googletest\msvc\gtest.vcxproj"> | ||
<ProjectReference Include="..\googletest\googletest\msvc\gtest.vcxproj"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to remove the msvc
folder for googletest as well
This change