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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get Non-windows platforms working #4861

Merged
merged 11 commits into from Jul 21, 2017
Merged

Get Non-windows platforms working #4861

merged 11 commits into from Jul 21, 2017

Conversation

bording
Copy link
Member

@bording bording commented Jul 19, 2017

This includes the changes needed to avoid anything that is platform-specific. Most of the changes are self-explanatory, for example dealing with path separator and line ending differences.

There are a few that could benefit from some more detail:

Marshal.SizeOf

The problem with the StructConventionsTest was that for the LogicalAddress struct, instead of showing a size, it instead had an error like Type cannot be marshaled as an unmanaged structure; no meaningful size or offset can be computed. On windows, calling Marshal.SizeOf on a struct with a reference field works fine, but not on other platforms. I found https://github.com/dotnet/corefx/issues/20442 describing the exact problem, and that it's by design that it doesn't work, because a reference field marshals as IUnknown, which is a windows-specific COM interface. So, on other platforms it throws instead. I adjusted the test to skip the size check if the struct has a reference field and indicate that it was skipped.

On a side note, the use of Marshal.SizeOf isn't really correct for how we're trying to use it in the test. What we really want here is the managed size of the struct, but the CLR specifically doesn't allow you to find that out. So instead, we're determining the unmanaged size of the struct if we were going to marshal it to native code. However, that size isn't guaranteed to be the same as the size in managed memory!

CopyLocalLockFileAssemblies

At first, I ran into a problem where dotnet test could not find any tests, but that was because of the differences in the locations of the NuGet cache between machines and the fact that we can't build on non-windows currently. Setting the CopyLocalLockFileAssemblies property works around this by making sure all the dependencies are in the build output folder instead of needing to be loaded from the cache.

ApprovalTests

The tests that rely on the ApprovalTests library were failing with an error about COM not being supported, but that turned out to be because of our chosen reporters. After switching to the NUnit reporter, I was still seeing failures because ApprovalTests couldn't find the *.approved.txt files. Looking into the ApprovalTests code, it uses some stack-walking code to try and figure out the location of the file. That doesn't seem to be working properly on linux, so the tests fail.

I looked around for another approval tests library, and found https://github.com/droyad/Assent. After building locally to deal with the lack of strong-naming in the official package, I found that I was running into the same problem: it couldn't find the *.approved.txt files. Instead of stack-walking, it uses the CallerFilePath attribute to get the file name, but this path is baked in at compile-time... so since we aren't building on linux, the paths are all wrong and it still can't find them! 馃槩

Ultimately, I decided to make the tests Assert.Ignore if they're running on a non-windows platform for now. If we can get to a point where we can actually build on linux, then we should take another look at Assent.


With these changes, I can now successfully run all tests on linux locally. 馃帀

@bording bording changed the title [WIP] Get Non-windows platforms working Get Non-windows platforms working Jul 19, 2017
}

static void InspectSizeOfStruct(Type type, List<string> violatedRules)
{
try
{
var sizeOf = Marshal.SizeOf(type);
if (IsLargerThanSixteenBytes(sizeOf))
var size = Marshal.SizeOf(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you share some background on the Marshal.SizeOf differences across the platforms?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the root comment to include information on the Marshal.SizeOf differences.

@@ -22,6 +22,11 @@ public class PipelineTests
[Test]
public async Task ShouldExecutePipeline()
{
if (Environment.OSVersion.Platform != PlatformID.Win32NT)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to write an attribute for that check?

Copy link
Member Author

Choose a reason for hiding this comment

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

NUnit has a Platform attribute that I tried to use first, bu it doesn't exist in their current netstandard assemblies. I suspect it's because it relies on the above code behind the scenes, and that API was added back to .NET Core in 2.0.

@bording
Copy link
Member Author

bording commented Jul 20, 2017

I've also verified that the tests run on macOS as well.

@timbussmann timbussmann merged commit 3f85a3d into develop Jul 21, 2017
@timbussmann timbussmann deleted the non-windows branch July 21, 2017 06:43
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.

None yet

2 participants