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

#71252 disable AtanPiTest for android x86 #71256

Closed
wants to merge 2 commits into from

Conversation

mkhamoyan
Copy link
Member

@mkhamoyan mkhamoyan commented Jun 24, 2022

Disabling AtanPiTest on android x86 due to #71252

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned mkhamoyan Jun 24, 2022
@mkhamoyan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan requested a review from steveisok June 24, 2022 13:30
@steveisok
Copy link
Member

/cc @tannergooding

@@ -1453,7 +1453,7 @@ public static void AsinPiTest(double value, double expectedResult, double allowe
AssertExtensions.Equal(+expectedResult, double.AsinPi(+value), allowedVariance);
}

[Theory]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotAndroid), nameof(PlatformDetection.IsNotX86Process))]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is being disabled due to #71252? We typically use [ActiveIssue(...)] for that. If that's not feasible because it doesn't support the exact condition, you can at least add the issue link as a comment next to the attribute. Otherwise it's likely we'll miss this in the future and it'll remain permanently disabled.

@tannergooding
Copy link
Member

tannergooding commented Jun 24, 2022

As per the comment by @drewnoakes on #71252, this is very likely a bug in Mono.

We have existing tests validating Math.Atan(-0.0) returns -0.0.

Likewise, -0.0 / PositiveNonZero (e.g. -0.0 / Math.PI) is -0.0. More specifically the result of x / y for a positive or negative zero, is 0 with the sign taken as an xor of the signs of x and y. So (and extend to any -y or +y rather than -1/+1):

  • -0 / -1 = +0
  • -0 / +1 = -0
  • +0 / -1 = -0
  • +0 / +1 = +0

Therefore Math.Atan(-0.0) / Math.PI which is the effective current implementation of double.AtanPi(double x) should be -0.0. This is the result on RyuJIT (and required answer per the IEEE 754 spec), so this is likely an error specific to Mono (potentially to Mono Android/x86).

@mkhamoyan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@mkhamoyan
Copy link
Member Author

Closing this as Atan2PiTest is already changed only for 64bit by this commit

@mkhamoyan mkhamoyan closed this Jun 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 2022
@mkhamoyan mkhamoyan deleted the 71252_disable_test branch August 19, 2022 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants