[POC] Add native tests #201

RassK · 2021-07-09T15:27:46Z

Adds native tests & CI support (GH Actions). Building via Nuke.

What's next:

Seems Linux / MacOS unit tests have been missing all time. We should look into adding these.
Remove workflow.yml when managed layer is covered.
Create target to ensure all autogenerated parts are committed to git (by running git diff --exit-code target)
Investigate OTEL_TRACE_LOG_DIRECTORY defaults and hardcoded parts.
Native lib output has still DD naming in Linux / MacOS. Correct in Windows.
Recheck .gitignore for Linux / MacOS

RassK · 2021-07-12T10:42:22Z

test/Datadog.Trace.ClrProfiler.Native.Tests/clr_helper_test.cpp

@@ -9,6 +9,8 @@ class CLRHelperTest : public ::CLRHelperTestBase {};

 TEST_F(CLRHelperTest, EnumeratesTypeDefs) {
  std::vector<std::wstring> expected_types = {
+      L"Microsoft.CodeAnalysis.EmbeddedAttribute",


@pjanotti Any idea what required these additional attributes? At first I thought it's GlobalSuppressions.cs, but then the solution already references linked solution level GlobalSuppressions. + Something turned on code analysis for dependency samples that wasn't required in previous commit.

CC - @pellared @dszmigielski

By the namespace this is an attribute type added by Roslyn, but I don't know its purpose. Can you point me to the commit in which wasn't required?

I think it was the first commit (7bce5a0) then nuke was added (d8b7e40) and additional attributes appeared.

@RassK from a quick check that attribute is added to netstandard1.0 and netstandard2.0 assemblies - I'm not sure but perhaps this was not added with some older version of Roslyn. Another possibility is that they were always there but the test didn't error when they were not listed (although the macro seems to imply equality).

dszmigielski

That's a lot of code, but I would love to have it merged ASAP. There is so much overlap on my task with what you have already done in terms of nuke setup, that doing that once again is just waste of time. If something is missing/wrong we can work on improving that along the way, but at least all of us would work on the same base.

build/nuke/Build.cs

pellared

Reviewed code related to Nuke.
I still need to review everything under test path

README.md

pellared · 2021-07-19T09:34:04Z

build_poc.sh

@@ -0,0 +1,62 @@
+#!/bin/bash


Can we rewritte this logic to Nuke?

yep, that's the second Todo after managed layer is nuked.

Datadog.Trace.Native.sln

build/nuke/Build.Steps.Linux.cs

build/nuke/Extensions/DotNetSettingsExtensions.cs

build/nuke/Build.Steps.Linux.cs

build/nuke/Build.Steps.cs

build/nuke/Build.cs

test/Datadog.Trace.ClrProfiler.Native.Tests/Datadog.Trace.ClrProfiler.Native.Tests.vcxproj

test/test-applications/instrumentation/dependency-libs/Samples.ExampleLibrary/Class1.cs

pellared · 2021-07-19T18:02:11Z

.github/workflows/ci.yml

@@ -0,0 +1,44 @@
+# ------------------------------------------------------------------------------


It would be great to add a target with executes git diff --exit-code to ensure that someone has not forgotten to commit the autogenerated code.

That's a great idea, I think it can be a task of it's own. Will add to todo.

pellared · 2021-07-19T18:04:49Z

docs/DEVELOPING.md

+To build using specific target run:
+
+```cmd
+dotnet nuke --target TargetNameHere


PS. dotnet nuke TargetNameHere works as well and this is what I use from commandline

Thinking about changing cmd line to:
dotnet nuke [--target] TargetNameHere

Wondering if it's understandable?

🤷 For me the output of dotnet nuke help is enough and I do not need this fragment 😉

Since it is "TargetNameHere" I think that the optional parameter doesn't add value.

docs/DEVELOPING.md

pjanotti · 2021-07-19T23:11:01Z

build.cmd

@@ -0,0 +1,7 @@
+:; set -eo pipefail


These lines seem like a left-over.

It is some kind of hack to make .cmd work on Linux and MacOS. I do not know how it works... But I think it does 😉

Magic 😮 - I needed to chmod it to try it.

On MacOS it also builds a bunch of files that are not ignored. @RassK could check if the .gitignore needs some updates?

pjanotti · 2021-07-19T23:54:22Z

test/Datadog.Trace.ClrProfiler.Native.Tests/clr_helper_test.cpp

@@ -9,6 +9,8 @@ class CLRHelperTest : public ::CLRHelperTestBase {};

 TEST_F(CLRHelperTest, EnumeratesTypeDefs) {
  std::vector<std::wstring> expected_types = {
+      L"Microsoft.CodeAnalysis.EmbeddedAttribute",


By the namespace this is an attribute type added by Roslyn, but I don't know its purpose. Can you point me to the commit in which wasn't required?

pellared · 2021-07-20T13:37:48Z

Moreover, I remember that @pjanotti was asking to reorganize in a way to have the productions code, test code, and sample code closer to each other.

Any thoughts about this?

pellared

I think this PR is a state that is worth merging.

pjanotti

LGTM

pjanotti · 2021-07-20T17:48:35Z

Moreover, I remember that @pjanotti was asking to reorganize in a way to have the productions code, test code, and sample code closer to each other.

Any thoughts about this?

@RassK @pellared we really should look into that but more related to instrumentations (there are some challenges but it should be possible).

RassK added 2 commits July 9, 2021 18:25

Add native tests

7bce5a0

Add nuke for windows

d8b7e40

RassK commented Jul 12, 2021

View reviewed changes

dszmigielski approved these changes Jul 12, 2021

View reviewed changes

RassK added 2 commits July 12, 2021 14:57

Nuke publish linux profiler

d211f11

fix scripts and add ci

0803a21

RassK force-pushed the native-tests branch from 614c3d5 to 0803a21 Compare July 12, 2021 14:22

fix file permissions

2c763d6

dszmigielski reviewed Jul 12, 2021

View reviewed changes

build/nuke/Build.cs Show resolved Hide resolved

RassK added 3 commits July 13, 2021 14:10

Add nuke MacOS steps

884ff49

Add missing script for linux

592e902

Fix Linux native lib name

d3c61e6

RassK force-pushed the native-tests branch from c93442f to d3c61e6 Compare July 13, 2021 11:54

RassK added 3 commits July 13, 2021 15:37

Add dotnet tool manifest

18c8aed

Fix macos native profiler lib name

f4f4dc1

update nuke schema

5a982cf

RassK marked this pull request as ready for review July 13, 2021 12:48

RassK requested a review from a team July 13, 2021 12:48

RassK changed the title ~~[WIP|POC] Add native tests~~ [POC] Add native tests Jul 13, 2021

RassK requested a review from dszmigielski July 13, 2021 12:51

Update docs about build

f9248d0

pellared reviewed Jul 19, 2021

View reviewed changes

test/Datadog.Trace.ClrProfiler.Native.Tests/Datadog.Trace.ClrProfiler.Native.Tests.vcxproj Outdated Show resolved Hide resolved

pellared reviewed Jul 19, 2021

View reviewed changes

test/test-applications/instrumentation/dependency-libs/Samples.ExampleLibrary/Class1.cs Outdated Show resolved Hide resolved

RassK added 6 commits July 19, 2021 18:40

Update docs

38bf557

fix gitignore

73a8881

refactor targets

874c38b

remove overleft sln

8537118

Remove GitLab helpers

d9806ab

Cleanup unused methods

f8927ab

Move developer notes to docs

171ac20

pellared reviewed Jul 19, 2021

View reviewed changes

pjanotti reviewed Jul 19, 2021

View reviewed changes

RassK added 6 commits July 20, 2021 13:14

remove incorrect duplicated directory

3fa24f9

fix scripts and gitignore

9148cac

Update vcxproj tools version

eab859b

fix example doc for LazyPathExecutableAttribute

82d9bc5

Cleanup & formatting

f88b52d

fix log path script

610e4b5

pellared approved these changes Jul 20, 2021

View reviewed changes

pjanotti approved these changes Jul 20, 2021

View reviewed changes

pjanotti merged commit 6965788 into open-telemetry:poc-otel-sdk Jul 20, 2021

RassK deleted the native-tests branch September 15, 2021 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Add native tests #201

[POC] Add native tests #201

RassK commented Jul 9, 2021 •

edited

Loading

RassK Jul 12, 2021 •

edited

Loading

pjanotti Jul 19, 2021

RassK Jul 20, 2021

pjanotti Jul 20, 2021 •

edited

Loading

dszmigielski left a comment

pellared left a comment •

edited

Loading

pellared Jul 19, 2021

RassK Jul 19, 2021

pellared Jul 19, 2021

RassK Jul 20, 2021

pellared Jul 19, 2021

RassK Jul 20, 2021

pellared Jul 20, 2021 •

edited

Loading

pjanotti Jul 20, 2021

pjanotti Jul 19, 2021

pellared Jul 20, 2021

pjanotti Jul 20, 2021

pjanotti Jul 20, 2021

pjanotti Jul 19, 2021

pellared commented Jul 20, 2021

pellared left a comment

pjanotti left a comment

pjanotti commented Jul 20, 2021

		@@ -0,0 +1,44 @@
		# ------------------------------------------------------------------------------

[POC] Add native tests #201

[POC] Add native tests #201

Conversation

RassK commented Jul 9, 2021 • edited Loading

RassK Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

dszmigielski left a comment

Choose a reason for hiding this comment

pellared left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared commented Jul 20, 2021

pellared left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

pjanotti commented Jul 20, 2021

RassK commented Jul 9, 2021 •

edited

Loading

RassK Jul 12, 2021 •

edited

Loading

pjanotti Jul 20, 2021 •

edited

Loading

pellared left a comment •

edited

Loading

pellared Jul 20, 2021 •

edited

Loading