Skip to content
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

Bump test projects up to .NET 4.5.2 #7607

Closed
wants to merge 6 commits into from
Closed

Bump test projects up to .NET 4.5.2 #7607

wants to merge 6 commits into from

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Feb 14, 2017

@@ -3,6 +3,7 @@
<PropertyGroup>
<DefineConstants>$(DefineConstants);CSPROJ</DefineConstants>
<DefineConstants Condition=" '$(TargetFrameworkMoniker)' == '.NETFramework,Version=v4.5.1' ">$(DefineConstants);NET451</DefineConstants>
<DefineConstants Condition=" '$(TargetFrameworkMoniker)' == '.NETFramework,Version=v4.5.2' ">$(DefineConstants);NET452</DefineConstants>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure whether this will be necessary after project.json files are removed. But, for now best to cover both cases…

@@ -33,7 +33,7 @@
"Microsoft.CSharp": "4.4.0-*"
}
},
"net451": {
"net452": {
Copy link
Member Author

Choose a reason for hiding this comment

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

@natemcmaster bit of a concern that I needed to change this project. Without the change, CI runs failed with missing symbols in test projects because the dependencies listed below weren't brought in. Is that a bug in transitive references?

With this change, I will also need to change other src projects that reference this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be. We should try this on csproj and see if it works better there (#7542)

@dougbu dougbu mentioned this pull request Feb 14, 2017
- aspnet/Testing#248
- xUnit no longer supports .NET 4.5.1
- does Microsoft.EntityFrameworkCore.Specification.Tests package ship to customers?
- require System.Xml framework assembly and that's not available xplat
- will have to wait for migration from project.json
@dougbu
Copy link
Member Author

dougbu commented Feb 14, 2017

@bricelam the CI failures on Windows seem to be fairly random -- SQL timeouts. But the failures on OSX and Linux are real and potentially a blocker. Microsoft.EntityFrameworkCore.InMemory.FunctionalTests and two other projects fail to compile because these platforms have no System.Xml.dll reference assemblies. I've tried a couple of things and have not improved matters.

Options:

  1. Forget this PR and address .NET 4.5.1 test runs fail with "EXEC : warning : No test is available" aspnet/Testing#248 as part of or just after Upgrade to VS 2017 #7542. This works because none of the test projects will attempt to build for desktop .NET except on Windows.
  2. Remove the net451 target framework from the test/Microsoft.EntityFrameworkCore.InMemory.FunctionalTests/project.json file and do the same for the other two projects and all test projects that depend on them. This unfortunately would affect most of the test projects on Windows too. This would not be 🆗 unless we test using the .csproj test projects on Windows.
  3. Work around the issue some other way. Since these projects build when targeting .NET 4.5.1 on these machines and .NET 4.5.2 should be treated as a superset, I'm not sure exactly what's going on.

Thoughts?

@bricelam
Copy link
Contributor

I opt for number one. I'll update that PR, and investigate any failures.

@bricelam bricelam closed this Feb 14, 2017
@dougbu dougbu deleted the dougbu/no.net451 branch February 14, 2017 18:47
@dougbu
Copy link
Member Author

dougbu commented Feb 14, 2017

Thanks @bricelam 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants