C#: Dotnet test tracer improvements. #13387

michaelnebel · 2023-06-06T15:23:16Z

It turns out that the the tracer (unconditionally) injects the property -p:UseSharedCompilation=false when dotnet test is executed.
However, if the dotnet test is applied to a library or executable it results in a dotnet error, when the property is set is added as it is not valid in this context.

In this PR we update the LUA tracer code, such that -p:UseSharedCompilation=false is not injected when dotnet test is applied to a library or executable.

…r dll.

csharp/tools/tracing-config.lua

csharp/ql/lib/change-notes/2023-06-06-dotnettest.md

csharp/ql/integration-tests/posix-only/dotnet_test/test.py

csharp/tools/tracing-config.lua

Co-authored-by: Michael B. Gale <[email protected]>

hvitved · 2023-06-07T12:00:33Z

csharp/tools/tracing-config.lua

@@ -48,6 +49,14 @@ function RegisterExtractorPack(id)
                    dotnetRunNeedsSeparator = true
                    dotnetRunInjectionIndex = i + 1
                end
+                if arg == 'test' then
+                    testMatch = true


How about

adding match = true here,

remove and not testMatch on line 38,

replace testMatch = false on line 58 with match = false; break, and

replace match or testMatch on line 74 with match?

Yes, we can also do that if we accept that the change also applies to dotnet run. It appears that dotnet run can also be invoked with a dll or exe file, but in this case we don't get a failure if the -p:UseSharedCompilation=false is provided.

So dotnet run when provided a dll or exe will not do any compilation?

Sorry, I was maybe not clear: I intended to keep the testMatch variable, so specifically on this line it would say

testMatch = true match = true

and then keep the check if testMatch and (arg:match('%.exe$') or arg:match('%.dll')) instead of just if arg:match('%.exe$') or arg:match('%.dll')

So dotnet run when provided a dll or exe will not do any compilation?

By closer inspection, that part is actually unclear as it is not documented as a part of dotnet run.
We will make sure to narrow the fix such that it only applies to dotnet test (which was a misunderstanding between @hvitved and me)

adityasharad

Some high-level suggestions.

adityasharad · 2023-06-06T15:26:23Z

csharp/tools/tracing-config.lua

+                    match = true
+                end
+                -- for `dotnet test`, we should not append `-p:UseSharedCompilation=false` to the command line
+                -- if a library or executable is being provided as an argument.


Good comment. It would be useful to include why: the arguments get passed in this case to vstest rather than msbuild.

Makes sense - I will extended the description and state that when an exe or dll is provided the call is forwarded to vstest. Thank you!

adityasharad · 2023-06-06T15:27:05Z

csharp/ql/integration-tests/posix-only/dotnet_test/test.py

 check_diagnostics()
+
+# Explicitly build and then run tests.
+run_codeql_database_create(['dotnet clean', 'rm -rf test-db', 'dotnet build -o myout', 'dotnet test myout/dotnet_test.dll'], test_db="test2-db", lang="csharp")


Is it feasible to have a similar test with .exe?

Yes, I will make another test.

adityasharad · 2023-06-07T14:50:22Z

csharp/tools/tracing-config.lua

@@ -48,6 +49,14 @@ function RegisterExtractorPack(id)
                    dotnetRunNeedsSeparator = true
                    dotnetRunInjectionIndex = i + 1
                end
+                if arg == 'test' then
+                    testMatch = true


So dotnet run when provided a dll or exe will not do any compilation?

hvitved

Let's do a DCA run before merging.

michaelnebel · 2023-06-08T07:53:28Z

Let's do a DCA run before merging.

Yes. I also want to add another test - working on it right now.

… get the UseSharedCompilation=false flag forwarded.

michaelnebel · 2023-06-08T08:21:06Z

Let's do a DCA run before merging.

Yes, I will start it now.

jaredpar · 2023-06-12T14:59:50Z

csharp/ql/lib/change-notes/2023-06-06-dotnettest.md

+---
+category: minorAnalysis
+---
+* C#: Analysis of the `dotnet test` command supplied with a `dll` or `exe` file as argument no longer fails due to the addition of an erroneous `-p:SharedCompilation=false` argument.


Should be -p:UseSharedCompilation not -p:SharedCompilation

Yes, thank you!!!
Will be fixed here: #13440

The release process has already been started, so we can't update the change note (since it is only a minor inaccuracy we don't want to stop the release to incorporate the change note modification).
Closing the related PR.

smitpatel · 2023-06-22T18:42:02Z

Which release this fix will be in? We are still seeing issue in our pipelines.

michaelnebel · 2023-06-23T06:31:58Z

Which release this fix will be in? We are still seeing issue in our pipelines.

This should be in release 2.13.4. Is that the one you are using?

smitpatel · 2023-06-23T07:27:48Z

@michaelnebel - Thank you. I will check the version we are using and figure out upgrade path.

michaelnebel added 2 commits June 6, 2023 16:53

C#: Add dotnet test that targets dll.

0f010af

C#: Only inject the shared compilation flag, if argument is not exe o…

4dae7ad

…r dll.

github-actions bot added the C# label Jun 6, 2023

hvitved reviewed Jun 7, 2023

View reviewed changes

csharp/tools/tracing-config.lua Outdated Show resolved Hide resolved

michaelnebel force-pushed the csharp/dotnettest branch from 9a98e98 to 2ab5f41 Compare June 7, 2023 07:55

github-actions bot added the documentation label Jun 7, 2023

michaelnebel marked this pull request as ready for review June 7, 2023 08:39

michaelnebel requested a review from a team as a code owner June 7, 2023 08:39

michaelnebel requested a review from hvitved June 7, 2023 08:40

mbg reviewed Jun 7, 2023

View reviewed changes

hvitved reviewed Jun 7, 2023

View reviewed changes

csharp/tools/tracing-config.lua Outdated Show resolved Hide resolved

michaelnebel added 2 commits June 7, 2023 12:43

C#: Address review comments.

5c9b0b9

C#: Add change note.

3eb3178

michaelnebel force-pushed the csharp/dotnettest branch from 1210544 to 3eb3178 Compare June 7, 2023 10:43

C#: Better change note.

d4d571e

Co-authored-by: Michael B. Gale <[email protected]>

michaelnebel requested review from mbg and hvitved June 7, 2023 10:49

hvitved reviewed Jun 7, 2023

View reviewed changes

C#: Address review comments.

f9c890b

michaelnebel force-pushed the csharp/dotnettest branch from c7ddf46 to f9c890b Compare June 7, 2023 12:54

tamasvajk mentioned this pull request Jun 7, 2023

C#: Implement correct behavior for dotnet build tracing #9705

Merged

adityasharad reviewed Jun 7, 2023

View reviewed changes

C#: Address review comments.

65e6515

hvitved previously approved these changes Jun 8, 2023

View reviewed changes

michaelnebel dismissed hvitved’s stale review via 7c15302 June 8, 2023 07:59

C#: Add MSTEST test project and check that the call to vstest doesn't…

2fece9d

… get the UseSharedCompilation=false flag forwarded.

michaelnebel force-pushed the csharp/dotnettest branch from 7c15302 to 2fece9d Compare June 8, 2023 08:01

michaelnebel requested a review from hvitved June 8, 2023 08:20

hvitved approved these changes Jun 8, 2023

View reviewed changes

mbg approved these changes Jun 8, 2023

View reviewed changes

hvitved merged commit a896be7 into github:main Jun 8, 2023

jaredpar reviewed Jun 12, 2023

View reviewed changes

michaelnebel deleted the csharp/dotnettest branch June 13, 2023 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Dotnet test tracer improvements. #13387

C#: Dotnet test tracer improvements. #13387

michaelnebel commented Jun 6, 2023 •

edited

Loading

hvitved Jun 7, 2023

michaelnebel Jun 7, 2023 •

edited

Loading

adityasharad Jun 7, 2023

hvitved Jun 7, 2023

michaelnebel Jun 8, 2023

adityasharad left a comment

adityasharad Jun 6, 2023

michaelnebel Jun 8, 2023

adityasharad Jun 6, 2023

michaelnebel Jun 8, 2023

adityasharad Jun 7, 2023

hvitved left a comment

michaelnebel commented Jun 8, 2023

michaelnebel commented Jun 8, 2023

jaredpar Jun 12, 2023

michaelnebel Jun 13, 2023

michaelnebel Jun 14, 2023

smitpatel commented Jun 22, 2023

michaelnebel commented Jun 23, 2023

smitpatel commented Jun 23, 2023

C#: Dotnet test tracer improvements. #13387

C#: Dotnet test tracer improvements. #13387

Conversation

michaelnebel commented Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

michaelnebel Jun 7, 2023 • 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

adityasharad left a 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

hvitved left a comment

Choose a reason for hiding this comment

michaelnebel commented Jun 8, 2023

michaelnebel commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Jun 22, 2023

michaelnebel commented Jun 23, 2023

smitpatel commented Jun 23, 2023

michaelnebel commented Jun 6, 2023 •

edited

Loading

michaelnebel Jun 7, 2023 •

edited

Loading