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

[linux-bionic] Skip failing NTLM tests #92830

Closed
wants to merge 1 commit into from

Conversation

steveisok
Copy link
Member

Some NTLM tests are still running and failing even though libSystem.Net.Security.Native isn't being built for linux-bionic. This PR skips them all.

Some NTLM tests are still running and failing even though libSystem.Net.Security.Native isn't being built for linux-bionic. This PR skips them all.
@ghost ghost assigned steveisok Sep 29, 2023
@steveisok steveisok marked this pull request as ready for review September 29, 2023 18:45
@ghost
Copy link

ghost commented Sep 29, 2023

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

Issue Details

Some NTLM tests are still running and failing even though libSystem.Net.Security.Native isn't being built for linux-bionic. This PR skips them all.

Author: steveisok
Assignees: steveisok
Labels:

area-System.Net.Security

Milestone: -

@steveisok
Copy link
Member Author

/azp run runtime-linuxbionic

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Sep 29, 2023

should we include the managed implementation? That should just work without GSSAP AFAIK.
cc: @filipnavara

@steveisok
Copy link
Member Author

should we include the managed implementation? That should just work without GSSAP AFAIK. cc: @filipnavara

Ah, yes, I think we should.

@filipnavara
Copy link
Member

We should but there was not any product-level check that we are running on linux-bionic.

@am11
Copy link
Member

am11 commented Sep 29, 2023

We should but there was not any product-level check that we are running on linux-bionic.

It is possible to test:

if (RuntimeInformation.RuntimeIdentifier.StartsWith("linux-bionic")) ForBionic();
else if (RuntimeInformation.RuntimeIdentifier.StartsWith("linux-musl")) ForMusl();
else ForGlibC();

@filipnavara
Copy link
Member

filipnavara commented Sep 30, 2023

It is possible to test:

Hmm. I though there's a reason why the test check is implemented differently. If RuntimeInformation.RuntimeIdentifier returns the correct information then we likely just need to modify this condition:

OperatingSystem.IsMacOS() || OperatingSystem.IsIOS() || OperatingSystem.IsMacCatalyst();

and add || (OperationSystem.IsLinux() && RuntimeInformation.RuntimeIdentifier.StartsWith("linux-bionic")).

@karelz karelz added this to the 9.0.0 milestone Oct 24, 2023
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Please solve the merge conflict.

I am requesting changes mostly so this PR does not show up at the next Stale PRs email ;)

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 3, 2023
@steveisok
Copy link
Member Author

For whatever reason, this doesn't appear to be failing in CI any longer.

@steveisok steveisok closed this Nov 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants