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.

Sign up for GitHub

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

Jump to bottom

C#: Dotnet test tracer improvements. #13387

Merged
merged 8 commits into from
Jun 8, 2023
Merged

C#: Dotnet test tracer improvements. #13387

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0f010af
C#: Add dotnet test that targets dll.
michaelnebel Jun 6, 2023
4dae7ad
C#: Only inject the shared compilation flag, if argument is not exe o…
michaelnebel Jun 6, 2023
5c9b0b9
C#: Address review comments.
michaelnebel Jun 7, 2023
3eb3178
C#: Add change note.
michaelnebel Jun 7, 2023
d4d571e
C#: Better change note.
michaelnebel Jun 7, 2023
f9c890b
C#: Address review comments.
michaelnebel Jun 7, 2023
65e6515
C#: Address review comments.
michaelnebel Jun 8, 2023
2fece9d
C#: Add MSTEST test project and check that the call to vstest doesn't…
michaelnebel Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion csharp/ql/integration-tests/posix-only/dotnet_test/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
from create_database_utils import *
from diagnostics_test_utils import *

run_codeql_database_create(['dotnet test'], db=None, lang="csharp")
# Implicitly build and then run tests.
run_codeql_database_create(['dotnet test'], test_db="test-db", lang="csharp")
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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will make another test.

check_diagnostics(test_db="test2-db")
4 changes: 4 additions & 0 deletions csharp/ql/lib/change-notes/2023-06-06-dotnettest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
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.
Copy link

Choose a reason for hiding this comment

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

Should be -p:UseSharedCompilation not -p:SharedCompilation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

13 changes: 12 additions & 1 deletion csharp/tools/tracing-config.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ function RegisterExtractorPack(id)
-- if that's `build`, we append `-p:UseSharedCompilation=false` to the command line,
-- otherwise we do nothing.
local match = false
local testMatch = false
local dotnetRunNeedsSeparator = false;
local dotnetRunInjectionIndex = nil;
local argv = compilerArguments.argv
Expand All @@ -37,7 +38,7 @@ function RegisterExtractorPack(id)
if (not match) then
Log(1, 'Dotnet subcommand detected: %s', arg)
end
if arg == 'build' or arg == 'msbuild' or arg == 'publish' or arg == 'pack' or arg == 'test' then
if arg == 'build' or arg == 'msbuild' or arg == 'publish' or arg == 'pack' then
match = true
break
end
Expand All @@ -48,6 +49,16 @@ function RegisterExtractorPack(id)
dotnetRunNeedsSeparator = true
dotnetRunInjectionIndex = i + 1
end
if arg == 'test' then
match = true
testMatch = true
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@michaelnebel michaelnebel Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

end
-- for `dotnet test`, we should not append `-p:UseSharedCompilation=false` to the command line
-- if an `exe` or `dll` is passed as an argument as the call is forwarded to vstest.
if testMatch and (arg:match('%.exe$') or arg:match('%.dll')) then
match = false
break
end
end
-- if we see a separator to `dotnet run`, inject just prior to the existing separator
if arg == '--' then
Expand Down