-
Notifications
You must be signed in to change notification settings - Fork 549
[msbuild/tests] Add tests for issue 3199 #5053
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
Conversation
- Fixes dotnet#3810: [msbuild] Add msbuild test for dotnet#3791: Skip reference assemblies passed to mtouch (dotnet#3810) - MSBuild tests now build the iOS and Mac test cases for issue 3199 that's using a nuget package as "PackageReference" which was causing some issues. With the fix disabled we get: ``` Errors and Failures: 1) Test Failure : Xamarin.iOS.Tasks.Issue3199.BuildTest #RunTarget-ErrorCount Could not AOT the assembly '/Users/vidondai/Documents/xi-master/xamarin-macios/msbuild/tests/Issue3199/obj/iPhone/Debug/mtouch-cache/3-Build/System.Runtime.CompilerServices.Unsafe.dll' Expected: 0 But was: 1 ``` and ``` "/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/Issue3199.csproj" (default target) (1) -> (_CompileToNative target) -> MMP : error MM3001: Could not AOT the assembly '/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/bin/Debug/Issue3199.app/Contents/MonoBundle/System.Runtime.CompilerServices.Unsafe.dll' [/Users/vidondai/Documents/xi-master/xamarin-macios/tests/common/mac/TestProjects/Issue3199/Issue3199.csproj] 63 Warning(s) 1 Error(s) ```
spouliot
left a comment
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.
Adding full projects (both iOS and Mac) won't scale, e.g. .if we have to change a single file (like Contents.json) for a new Xcode we'll end up with dozens of files to update.
Also it hides what's the real content of the test (without referring to the issue, i.e. being connected) to the point where it's not clear if a more simple (unit) test would be enough (and much more faster than building the projects).
What about adding a template directory (on iOS and one macOS) and symlinks to the unmodified files ?
|
TL;DR; I hear you but I believe what you're asking for is an enhancement request on its own (I'm happy to investigate) and this PR, as is, is how those things have always been tested. So I think it's worth merging it IMHO.
@spouliot I agree but you're questioning almost all msbuild tests with that reasoning. This is how things are done today (not that I like it), you add full projects and build them and yes it requires adding a bunch of redundant files. There might be a slightly better approach possible based on some Mac msbuild tests that are using a shared csproj but the fact we need to restore nugget packages might be a deal breaker. See @chamons comment in https://xamarinhq.slack.com/archives/C03CCJHCF/p1540827269024100. Note: The only msbuild tests that don't require full projects are the tasks tests that are more "classic" unit tests.
I could add a "test comment" to explain what it does or even rename it so it's clearer. |
|
@VincentDondain nothing personal :) Build and test times were not priorities until recently. Now that we're working on (both fixing and measuring) them it means further work must not undo our gains - which could be easy since testing msbuild has been unblocked. |
|
Build failure Test results1 tests failed, 0 tests skipped, 218 tests passed.Failed tests
|
([msbuild] Add msbuild test for #3791: Skip reference assemblies passed to mtouch #3810)
With the fix disabled we get:
and