Fix Nagler tests to run on Linux by willxie-eng · Pull Request #524 · microsoft/CLRInstrumentationEngine

willxie-eng · 2022-10-10T18:42:07Z

Summary

These changes fix InstrEngineTests.dll run on Linux.

Change Log

Fix XmlNode to correctly parse comment nodes with LibXML2
Fix InstrumentationMethod logging variable check since Linux has case-sensitive environment variables
Add Nagler config & .so & update DeploymentItems for tests
Update ProfilerHelper.cs to be OS-aware
Minor fixes to Nagler's CMake
Update InstrumentationEngineApi.cpp to log module load failures to stderrr
Update Ubuntu DockerFile to install net7 dotnet
Update PR.yaml to run linux tests - https://dev.azure.com/ms/CLRInstrumentationEngine/_build/results?buildId=382640&view=results

Test Methodology

Local build & test

Date: Mon Oct 10 11:23:07 2022 -0700 Squashed commit of the following: commit 2292449 Author: William Xie <willxie@microsoft.com> Date: Fri Oct 7 14:30:06 2022 -0700 Fix up some last bugs commit 364ab15 Author: William Xie <willxie@microsoft.com> Date: Fri Oct 7 10:36:24 2022 -0700 Fix libxml2 handling of comment nodes commit 4bcbe1f Author: William Xie <willxie@microsoft.com> Date: Thu Oct 6 21:42:24 2022 -0700 Add timeout override commit 85b5661 Author: William Xie <willxie@microsoft.com> Date: Thu Oct 6 21:22:28 2022 -0700 Fix logging test commit 8ca135c Author: William Xie <willxie@microsoft.com> Date: Thu Oct 6 21:02:01 2022 -0700 Fix linux nagler tests commit e21c6db Author: William Xie <willxie@microsoft.com> Date: Thu Oct 6 11:33:38 2022 -0700 temporary changes

willxie-eng · 2022-10-10T18:43:03Z

TODO: update test.md #Resolved

In reply to: 1273686615

willxie-eng · 2022-10-10T18:46:53Z

+        public const string NaglerInstrumentationConfigX86BinPath = @"../../x86/NaglerInstrumentationMethod_x86.xml";
+        public const string NaglerInstrumentationMethodX64BinPath = @"../../x64/NaglerInstrumentationMethod_x64.dll";
+        public const string NaglerInstrumentationMethodX86BinPath = @"../../x86/NaglerInstrumentationMethod_x86.dll";
+        public const string LinuxInstrumentationEngineX64BinPath = @"../../../../out/Linux/bin/x64.Debug/ClrInstrumentationEngine/libInstrumentationEngine.so";


out/Linux/bin/x64.Debug

These aren't available during build

I wish unit tests supported conditionally ignoring tests. Maybe separate the tests into categories, and only run the Windows tests on Windows, and the other tests on other platforms?

We can potentially leverage the --filter option on dotnet test and rename some tests with a Windows prefix, but I'd like to make that change in a separate PR.

Another way to do this is, I think, deployment items can be entire directories. So, we could copy the files that we need to a particular location as part of the build, and then just deploy the directory.

Yeah, that's essentially the same thing. You can filter on categories as well. Something like this:

internal abstract class TestClassImpl { [TestMethod] void Method1() {...} } [TestClass] [Category("Windows")]; [DeploymentItem("WindowsFile", ...)] internal class WindowsTest : TestClassImpl {} [TestClass] [Category("Linux")]; [DeploymentItem("LinuxFile", ...)] internal class LinuxTest: TestClassImpl {}

willxie-eng · 2022-10-11T00:19:16Z

@microsoft-github-policy-service test

willxie-eng · 2022-10-11T00:19:32Z

@microsoft-github-policy-service agree company="Microsoft"

willxie-eng · 2022-10-11T00:21:33Z

@microsoft-github-policy-service agree

delmyers · 2022-10-11T15:49:03Z

+      targetFolder: $(Build.StagingDirectory)/binaries-windows-$(Configuration)/AnyCPU/net70/
+
+  - task: UseDotNet@2
+    displayName: 'Use .NET 7 sdk'


Should we test .NET 6 as well, since it is the LTS version? We could parameterize this job and run it twice. #Closed

Makes sense, I'll update

delmyers · 2022-10-11T17:41:38Z

+                    is32bitTest ? HostConfig32PathEnvName : HostConfig64PathEnvName,
+                    Path.Combine(PathUtils.GetAssetsPath(), string.Format(CultureInfo.InvariantCulture, "NaglerInstrumentationMethod_{0}.xml", bitnessSuffix)));
+            }
+            else // Linux


else // Linux

We don't build for FreeBSD. We should probably condition this to Linux, and MacOS #Resolved

delmyers · 2022-10-11T17:46:02Z

        [Timeout(TestConstants.TestTimeout)]
        public void TestRefStructs_Validate_RefStructWithRefField_Load()
        {
+            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) 


Instead of rewriting this code over and over, you can extend TestMethodAttribute:

internal class WindowsTestMethodAttribute: TestMethodAttribute { public override TestResult[] Execute(ITestMethod testMethod) { if (!RuntimeInformation.IsOsPlatform(OSPlatform.Windows)) { return new[] { new TestResult() { Outcome = UnitTestOutcome.Inconclusive, TestFailureException = new AssertInconclusiveException("Test only runs on Windows") }}; } return base.Execute(testMethod); } } ``` #Closed

willxie-eng · 2022-10-11T17:51:47Z

+#  - download: current
+#    patterns: |
+#      binaries-windows-$(Configuration)/AnyCPU/**
+#      binaries-linux-ubuntu-$(Configuration)/ClrInstrumentationEngine/**


removing; this was the new way to download pipeline artifacts but for some reason the files are inaccessible in subsequent tasks (file/directory not found errors). #Resolved

delmyers · 2022-10-11T18:00:33Z

 #define MOD_LOAD(_N) (GetModuleHandle(_N))
 #define SYM_LOAD(_M, _S) (GetProcAddress(_M, _S))
-
+#define GET_LAST_ERROR() \


GET_LAST_ERROR

You could just as easily make this a function, and it would be easier to understand:

void AssertLastError() { #ifdef PLATFORM_UNIX // linux/mac here #else // windows stuff #endif }

remove the macro

delmyers

🕐

willxie-eng · 2022-10-11T18:03:09Z

+      publishTestResults: true
+    condition: succeededOrFailed()
+
+  #- script: $(Agent.TempDirectory)/dotnet $(Pipeline.Workspace)/binaries-windows-Debug/AnyCPU/net70/InstrEngineTests.dll


#- script: $(Agent.TempDirectory)/dotnet $(Pipeline.Workspace)/binaries-windows-Debug/AnyCPU/net70/InstrEngineTests.dll

removing #Resolved

delmyers · 2022-10-11T22:32:41Z

+                psi.EnvironmentVariables.Add(
+                    HostConfig64PathEnvName,
+                    Path.Combine(PathUtils.GetAssetsPath(), "LinuxNaglerInstrumentationMethod.xml"));
+            }


nit: I think this could technically be in the NETCOREAPP as well.

true but it doesn't change the behavior so I'll leave it as-is.

delmyers

willxie-eng requested a review from a team as a code owner October 10, 2022 18:42

willxie-eng commented Oct 10, 2022

View reviewed changes

Update test.md and fix linux managed build

f224305

microsoft deleted a comment from microsoft-github-policy-service Bot Oct 11, 2022

Test PR build

b2fc789

willxie-eng mentioned this pull request Oct 11, 2022

CLA bot doesn't acknowledge agreement microsoft/ContributorLicenseAgreement#50

Closed

delmyers reviewed Oct 11, 2022

View reviewed changes

willxie-eng commented Oct 11, 2022

View reviewed changes

delmyers reviewed Oct 11, 2022

View reviewed changes

delmyers suggested changes Oct 11, 2022

View reviewed changes

willxie-eng commented Oct 11, 2022

View reviewed changes

PR comments

2bb0230

delmyers reviewed Oct 11, 2022

View reviewed changes

delmyers approved these changes Oct 11, 2022

View reviewed changes

willxie-eng merged commit e7020cd into main Oct 11, 2022

willxie-eng deleted the dev/willxie/naglerlinux branch October 11, 2022 22:48

willxie-eng mentioned this pull request Oct 27, 2022

Build NaglerInstrumentationMethod on Linux #361

Closed

Conversation

willxie-eng commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Log

Test Methodology

Uh oh!

willxie-eng commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willxie-eng Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

willxie-eng Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

willxie-eng commented Oct 11, 2022

Uh oh!

willxie-eng commented Oct 11, 2022

Uh oh!

willxie-eng commented Oct 11, 2022

Uh oh!

delmyers Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willxie-eng Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022 • edited by willxie-eng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willxie-eng Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

delmyers left a comment

Choose a reason for hiding this comment

Uh oh!

willxie-eng Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

willxie-eng Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delmyers left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

willxie-eng commented Oct 10, 2022 •

edited

Loading

willxie-eng commented Oct 10, 2022 •

edited

Loading

willxie-eng Oct 10, 2022 •

edited

Loading

delmyers Oct 11, 2022 •

edited

Loading

delmyers Oct 11, 2022 •

edited by willxie-eng

Loading

delmyers Oct 11, 2022 •

edited

Loading

willxie-eng Oct 11, 2022 •

edited

Loading

delmyers Oct 11, 2022 •

edited

Loading

willxie-eng Oct 11, 2022 •

edited

Loading

willxie-eng Oct 11, 2022 •

edited

Loading