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

#51371 fixed failing test in iossimulator #63877

Merged
merged 18 commits into from
Jan 27, 2022

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Jan 17, 2022

Fixes #51371

For CaseSensitive tests added check if they are running on ios/tvos_simulators or MacCatalyst.
Changed GetPipePath() and GetPath() for ios/tvos_simulator as they were too long and there was ArgumentOutOfRange exception

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 17, 2022
@ghost
Copy link

ghost commented Jan 17, 2022

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

Issue Details

null

Author: mkhamoyan
Assignees: -
Labels:

area-System.IO, community-contribution

Milestone: -

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-manual

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 63877 in repo dotnet/runtime

@simonrozsival
Copy link
Member

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dnfadmin
Copy link

dnfadmin commented Jan 17, 2022

CLA assistant check
All CLA requirements met.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok steveisok self-requested a review January 19, 2022 02:23
public void SearchPatternCaseSensitive()
{
if (RuntimeInformation.RuntimeIdentifier.StartsWith("iossimulator")
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea.

@@ -219,16 +219,10 @@ public void LongFileName()
Assert.False(File.Exists(path));
}

[ConditionalFact]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotRunningOnMacOS))]
[PlatformSpecific(CaseSensitivePlatforms)]
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to expand upon CaseSensitivePlatforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded IsCaseSensitiveOS.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-manual

@@ -232,6 +232,12 @@ public override void EndWrite(IAsyncResult asyncResult)

internal static string GetPipePath(string serverName, string pipeName)
{
if (RuntimeInformation.RuntimeIdentifier.StartsWith("iossimulator")
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the logic here, this file is not included in simulator builds

@@ -23,7 +23,7 @@ public abstract partial class FileCleanupTestBase : IDisposable
/// <summary>Initialize the test class base. This creates the associated test directory.</summary>
protected FileCleanupTestBase(string tempDirectory = null)
{
tempDirectory ??= Path.GetTempPath();
tempDirectory ??= PlatformDetection.IsCaseSensitiveOS ? Path.GetTempPath() : "/tmp/";
Copy link
Member

Choose a reason for hiding this comment

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

I would expect GetTempPath to work in all cases. Did you find otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For iossimulator I get exception in IOSServices.GetPath function rootPath is long and I see in logs [Assert.Equal() Failure\nExpected: 257\nActual: 248]] for several test cases. After this change exception is not happening.

Copy link
Member

Choose a reason for hiding this comment

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

Can you look at extending

public static readonly int MaxDirectory = 247;
? For OSX, I believe the max path length is 1024 and the max length of a file name is 255.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to 1016 (bigger values are throwing path too long exception) when it is not windows.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

if (RuntimeInformation.RuntimeIdentifier.StartsWith("iossimulator")
|| RuntimeInformation.RuntimeIdentifier.StartsWith("tvossimulator"))
{
return $"/tmp/{pipeName}";
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of simulators in UnixDomainSocketEndPoint there was ArgumentOutOfRangeException. I added this to make path length shorter because s_nativePathLength is 104 (in UnixDomainSocketEndPoint) and by previous implementation path length is 255.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that part. Find out in FileCleanupTestBase path was wrong for osx, updated that.

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

The windows failures look unrelated.

@mkhamoyan mkhamoyan merged commit 9fafc04 into dotnet:main Jan 27, 2022
@akoeplinger akoeplinger removed the community-contribution Indicates that the PR has been added by a community member label Jan 27, 2022
@danmoseley
Copy link
Member

Cryptography tests are sometimes hanging on Windows due to an OS issue. @bartonjs do we have a tracking issue or something?

@bartonjs
Copy link
Member

@danmoseley Ooh, we've started testing on Win11... how fancy :)

I haven't seen an issue in dotnet/runtime talking about it (this is the first mention I had that the problem was seen in CI, vs just on a few individuals' machines). I have the Windows OS bug for it, but since those aren't useful to non-employees I generally don't think it's appropriate to post them here.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2022
@mkhamoyan mkhamoyan deleted the 51371_failing_filesystem_tests branch August 19, 2022 12:45
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.IO.FileSystem.Tests Fails on iOS/tvOS
9 participants