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

[iOS] Remove unnecessary newline from CI script #67861

Merged
merged 20 commits into from
Aug 2, 2022
Merged

[iOS] Remove unnecessary newline from CI script #67861

merged 20 commits into from
Aug 2, 2022

Conversation

steveisok
Copy link
Member

A newline was incorrectly added to the script that executes xharness, causing it to return a 0 exit code even if there was a crash. This caused CI to report a pass no matter what.

System.IO.Tests was the only suite found where this had an impact.

Contributes to #67853

@ghost ghost assigned steveisok Apr 11, 2022
@ghost
Copy link

ghost commented Apr 11, 2022

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

Issue Details

A newline was incorrectly added to the script that executes xharness, causing it to return a 0 exit code even if there was a crash. This caused CI to report a pass no matter what.

System.IO.Tests was the only suite found where this had an impact.

Contributes to #67853

Author: steveisok
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Apr 11, 2022
Closed
Closed
Closed
Closed
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member

@steveisok are the test failures related or just flakiness? Asking as the PR is showing up on our infra board :)

@steveisok
Copy link
Member Author

@ViktorHofer The tvOS arm64 failures are related. Still need to go through them and create issues.

@steveisok steveisok requested a review from mkhamoyan May 31, 2022 13:39
@mkhamoyan
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Jul 26, 2022
Closed
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jul 27, 2022
Open
Open
mdh1418
mdh1418 reviewed Jul 27, 2022
View reviewed changes
eng/pipelines/runtime-extra-platforms-other.yml Show resolved Hide resolved
src/libraries/System.Formats.Cbor/tests/PropertyTests/CborPropertyTests.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.Compression.ZipFile/tests/ZipFileExtensions.ZipArchive.Extract.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.Compression.ZipFile/tests/ZipFileExtensions.ZipArchive.Extract.cs Outdated Show resolved Hide resolved
...time.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/PtrToStructureTests.cs Outdated Show resolved Hide resolved
src/libraries/tests.proj Outdated Show resolved Hide resolved
src/libraries/tests.proj Show resolved Hide resolved
src/libraries/System.Runtime.Extensions/tests/System/AppDomainTests.cs Outdated Show resolved Hide resolved
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Jul 29, 2022
Open
@steveisok
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418
mdh1418 approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Are we planning to alter _AfterBuildCommands to help avoid hitting another newline in the future as suggested by https://github.com/dotnet/runtime/pull/67861/files#r896851313? The rest looks good to me!

@steveisok
Copy link
Member Author

Are we planning to alter _AfterBuildCommands to help avoid hitting another newline in the future as suggested by https://github.com/dotnet/runtime/pull/67861/files#r896851313? The rest looks good to me!

Yes, in a follow up.

@steveisok steveisok merged commit aac729f into dotnet:main Aug 2, 2022
MichalStrehovsky
MichalStrehovsky reviewed Aug 2, 2022
View reviewed changes
src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
@@ -153,6 +153,7 @@ private static bool GetLinqExpressionsBuiltWithIsInterpretingOnly()
public static bool IsNotIntMaxValueArrayIndexSupported => s_largeArrayIsNotSupported.Value;

public static bool IsAssemblyLoadingSupported => !IsNativeAot;
public static bool IsNonBundledAssemblyLoadingSupported => !IsAssemblyLoadingSupported && !IsMonoAOT;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this condition.

Non-bundled assembly loading is supported when assembly loading is not supported and it's not Mono AOT?

This made the test using this run exclusively on NativeAOT. It's not supposed to run there. It's breaking the rolling CI.

I don't understand the specifics, but this probably should have extended the definition of IsAssemblyLoadingSupported instead. Or it shouldn't use the !. It doesn't make sense as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad. The ! needs to go away. I'll put up a fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

#73235

@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akoeplinger akoeplinger akoeplinger left review comments

@premun premun premun approved these changes

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@lambdageek lambdageek lambdageek approved these changes

@mdh1418 mdh1418 mdh1418 approved these changes

@simonrozsival simonrozsival Awaiting requested review from simonrozsival

@mkhamoyan mkhamoyan Awaiting requested review from mkhamoyan

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@steveisok steveisok

Labels
area-Infrastructure-mono os-tvos Apple tvOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@steveisok @ViktorHofer @mkhamoyan @lambdageek @akoeplinger @premun @MichalStrehovsky @mdh1418 @jeffhandley