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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUIC] Fix test for debug/release version of a library #93449

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

ManickaP
Copy link
Member

The originally used preexisting platform detection is for runtime not libraries.

Fixes #93404

@ghost
Copy link

ghost commented Oct 13, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The originally used preexisting platform detection is for runtime not libraries.

Fixes #93404

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@@ -103,6 +103,12 @@ public static bool IsPrivilegedProcess
public static bool IsReleaseRuntime => s_isReleaseRuntime.Value;
public static bool IsDebugRuntime => s_isDebugRuntime.Value;

private static readonly Lazy<bool> s_isReleaseLibraries = new Lazy<bool>(() => AssemblyConfigurationEquals("Release"));
private static readonly Lazy<bool> s_isDebugLibraries = new Lazy<bool>(() => AssemblyConfigurationEquals("Debug"));
Copy link
Member

@akoeplinger akoeplinger Oct 13, 2023

Choose a reason for hiding this comment

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

isn't this the same as s_isReleaseRuntime and s_isDebugRuntime ?

AssemblyConfigurationEquals checks typeof(string).Assembly so I think an easier solution would be to add an overload that takes the assembly there instead of IsDebuggable(Assembly assembly)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no, those are remnants, they shouldn't be here. The two bellow are important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Member

Choose a reason for hiding this comment

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

ok but why do you need to check DebuggableAttribute instead of adding an overload to the existing AssemblyConfigurationEquals?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can see in my tests, it returns the configuration of runtime, not the library and the attribute is not present for System.Net.Quic.dll:

Console.WriteLine(typeof(QuicConnection).Assembly.GetCustomAttribute<AssemblyConfigurationAttribute>() is null); // True

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, looks like we disable generating AssemblyConfiguration attributes for all libraries:

<!-- Libraries have never generated these attributes so don't let the SDK generate them. -->
<GenerateAssemblyConfigurationAttribute>false</GenerateAssemblyConfigurationAttribute>

@ManickaP
Copy link
Member Author

Failures are #92423

@ManickaP ManickaP merged commit 7e118e7 into dotnet:main Oct 13, 2023
103 of 109 checks passed
@ManickaP ManickaP deleted the mapichov/quic-fix-test branch October 13, 2023 10:34
Comment on lines +25 to 27
if (PlatformDetection.IsReleaseLibrary(typeof(QuicConnection).Assembly))
{
throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode.");
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicking, but since there's also 'checked' build possibility, I feel it might be better to check !IsDebugLibrary instead of IsReleaseLibrary (I see that implementation-wise this is the same, but it would make a bit more sense to me)

Suggested change
if (PlatformDetection.IsReleaseLibrary(typeof(QuicConnection).Assembly))
{
throw new SkipTestException("Retrieving SSL secrets is not supported in Release mode.");
if (!PlatformDetection.IsDebugLibrary(typeof(QuicConnection).Assembly))
{
throw new SkipTestException("Retrieving SSL secrets is supported only in Debug mode.");

Copy link
Member

@CarnaViire CarnaViire Oct 13, 2023

Choose a reason for hiding this comment

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

Baah, I spent too much time writing that 😂 you can disregard since this is merged already

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no "checked" version of library, that applies only to CLR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Net.Quic.Tests.MsQuicRemoteExecutorTests.SslKeyLogFile_IsCreatedAndFilled fails
4 participants