#51371 fixed failing test in iossimulator by mkhamoyan · Pull Request #63877 · dotnet/runtime

mkhamoyan · 2022-01-17T10:24:43Z

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 · 2022-01-17T10:24:49Z

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 · 2022-01-17T10:28:12Z

/azp run runtime-manual

azure-pipelines · 2022-01-17T10:28:17Z

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

simonrozsival · 2022-01-17T10:41:34Z

/azp run runtime-manual

azure-pipelines · 2022-01-17T10:41:47Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.IO.FileSystem/tests/File/Create.cs

dnfadmin · 2022-01-17T14:33:32Z

All CLA requirements met.

mkhamoyan · 2022-01-17T16:34:53Z

/azp run runtime-manual

azure-pipelines · 2022-01-17T16:35:07Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2022-01-19T02:25:37Z

src/libraries/System.IO.FileSystem/tests/Directory/GetFileSystemEntries_str_str.cs

-        [ActiveIssue("https://github.com/dotnet/runtime/issues/51371", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
        public void SearchPatternCaseSensitive()
        {
+            if (RuntimeInformation.RuntimeIdentifier.StartsWith("iossimulator")


Should we add a property to https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs and use it in the condition instead?

Yes, good idea.

src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs

steveisok · 2022-01-19T23:25:55Z

src/libraries/System.IO.FileSystem/tests/File/Create.cs


-        [ConditionalFact]
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotRunningOnMacOS))]
        [PlatformSpecific(CaseSensitivePlatforms)]


Would it make more sense to expand upon CaseSensitivePlatforms?

Expanded IsCaseSensitiveOS.

mkhamoyan · 2022-01-20T08:53:19Z

/azp run runtime-manual

marek-safar · 2022-01-21T08:25:01Z

src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs


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


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

steveisok · 2022-01-21T14:01:41Z

src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs

        protected FileCleanupTestBase(string tempDirectory = null)
        {
-            tempDirectory ??= Path.GetTempPath();
+            tempDirectory ??= PlatformDetection.IsCaseSensitiveOS ? Path.GetTempPath() : "/tmp/";


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

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.

Can you look at extending

runtime/src/libraries/System.IO.FileSystem/tests/PortedCommon/IOInputs.cs

Line 26 in 0240828

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.

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

mkhamoyan · 2022-01-21T14:56:43Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-01-21T14:57:06Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-01-24T12:06:15Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-01-24T12:06:34Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-01-25T15:55:18Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-01-25T15:55:38Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2022-01-25T20:25:21Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-01-25T20:25:40Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2022-01-26T18:38:16Z

src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs

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


I'm not sure why this is needed?

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.

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

…leCleanupTestBase.cs

mkhamoyan · 2022-01-26T20:49:36Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-01-26T20:49:56Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok

The windows failures look unrelated.

danmoseley · 2022-01-27T15:07:03Z

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

bartonjs · 2022-01-27T16:16:27Z

@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.

dotnet#51371 fixed failing test in iossimulator

c9e82e3

ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jan 17, 2022

akoeplinger reviewed Jan 17, 2022

View reviewed changes

src/libraries/System.IO.FileSystem/tests/File/Create.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem/tests/File/Create.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem/tests/File/Create.cs Show resolved Hide resolved

dotnet#51371 added skiptestexception

0428831

dotnet#51371 added space after reference

cfe4592

steveisok self-requested a review January 19, 2022 02:23

steveisok reviewed Jan 19, 2022

View reviewed changes

mkhamoyan added 2 commits January 19, 2022 18:34

dotnet#51371 fixed remaining failing tests

4b8fe65

dotnet#51371 whitespace issue

7a3053c

steveisok reviewed Jan 19, 2022

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs Outdated Show resolved Hide resolved

steveisok reviewed Jan 19, 2022

View reviewed changes

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs Outdated Show resolved Hide resolved

steveisok reviewed Jan 19, 2022

View reviewed changes

mkhamoyan marked this pull request as ready for review January 20, 2022 08:34

mkhamoyan requested review from MaximLipnin, akoeplinger, directhex, lateralusX, simonrozsival and steveisok January 20, 2022 08:35

dotnet#51371 removed extra spaces

5e2771d

marek-safar reviewed Jan 21, 2022

View reviewed changes

mkhamoyan added 2 commits January 21, 2022 11:18

dotnet#51371 removed pipePath change in windows file

6154087

dotnet#51371 Done minor changes

d989e86

steveisok reviewed Jan 21, 2022

View reviewed changes

dotnet#51371 extended maxdirectory for not windows

d457075

mkhamoyan requested a review from steveisok January 24, 2022 12:14

dotnet#51371 in case of windows don't change parameter IsCaseSensitiveOS

bdc06e9

dotnet#51371 changing maxdirectory for nonwindows

eddd2ca

steveisok reviewed Jan 26, 2022

View reviewed changes

dotnet#51371 removed from Pipestream the change and edited path in Fi…

2501ef2

…leCleanupTestBase.cs

mkhamoyan requested a review from steveisok January 26, 2022 20:49

mkhamoyan requested a review from MaximLipnin January 27, 2022 12:54

MaximLipnin approved these changes Jan 27, 2022

View reviewed changes

steveisok approved these changes Jan 27, 2022

View reviewed changes

mkhamoyan merged commit 9fafc04 into dotnet:main Jan 27, 2022

akoeplinger removed the community-contribution Indicates that the PR has been added by a community member label Jan 27, 2022

ghost locked as resolved and limited conversation to collaborators Feb 26, 2022

mkhamoyan deleted the 51371_failing_filesystem_tests branch August 19, 2022 12:45

Conversation

mkhamoyan commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 17, 2022

Uh oh!

mkhamoyan commented Jan 17, 2022

Uh oh!

azure-pipelines bot commented Jan 17, 2022

Uh oh!

simonrozsival commented Jan 17, 2022

Uh oh!

azure-pipelines bot commented Jan 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnfadmin commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkhamoyan commented Jan 17, 2022

Uh oh!

azure-pipelines bot commented Jan 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Jan 21, 2022

Uh oh!

azure-pipelines bot commented Jan 21, 2022

Uh oh!

mkhamoyan commented Jan 24, 2022

Uh oh!

azure-pipelines bot commented Jan 24, 2022

Uh oh!

mkhamoyan commented Jan 25, 2022

Uh oh!

azure-pipelines bot commented Jan 25, 2022

Uh oh!

mkhamoyan commented Jan 25, 2022

Uh oh!

azure-pipelines bot commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhamoyan commented Jan 26, 2022

Uh oh!

azure-pipelines bot commented Jan 26, 2022

Uh oh!

steveisok left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jan 27, 2022

Uh oh!

bartonjs commented Jan 27, 2022

Uh oh!

Reviewers

mkhamoyan commented Jan 17, 2022 •

edited

Loading

dnfadmin commented Jan 17, 2022 •

edited

Loading