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

Disabled System.Security.Cryptography.Tests for Android x64 and x86 #66833

Merged
merged 3 commits into from
Mar 19, 2022

Conversation

ilonatommy
Copy link
Member

Fixes #66831.

@ghost
Copy link

ghost commented Mar 18, 2022

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

Issue Details

Fixes #66831.

Author: ilonatommy
Assignees: ilonatommy
Labels:

area-System.Security

Milestone: -

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.

Thanks, looks good to me after fixing the paths and whether we want it to still run on Android Arm64.

src/libraries/tests.proj Outdated Show resolved Hide resolved
src/libraries/tests.proj Outdated Show resolved Hide resolved
src/libraries/tests.proj Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 18, 2022
@danmoseley danmoseley changed the title Disabled System.Security.Cryptography.Tests. Disabled System.Security.Cryptography.Tests for Android x64 and x86 Mar 18, 2022
@bartonjs
Copy link
Member

Disabling crypto tests on Android feels like a very heavy hammer. Especially since this is one of the areas where there's actually a lot of OS-specific code (so it's significantly more important to run the crypto tests on Android than it is to run, e.g. collections tests).

Are we running out of memory because there's a memory leak? If so, someone (with os-android experience) should investigate/fix it.

Is it just intermittent because it's down to test parallelization? Maybe disable test parallelization for the library (on Android).

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 18, 2022
@ilonatommy
Copy link
Member Author

For now I disabled only the ones that actually posed a problem, not all of them, taking into consideration @bartonjs's opinion. That's only a temporary solution till the fix won't be found. @bartonjs, @danmoseley, @mdh1418 do you know who might be able to deal with this issue?

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.

I'm not sure who is the go to person for a memory issue like this, but if we follow through with just disabling the problematic architectures and keeping the Arm64 in hopes that it would catch all possible regressions, the changes look good to me.

src/libraries/tests.proj Outdated Show resolved Hide resolved
@danmoseley
Copy link
Member

On our team we do not have equipment or expertise to investigate device specific issues. Mono team handles this. @mdh1418 who on your team would look?

@ilonatommy ilonatommy merged commit 3fc6dab into dotnet:main Mar 19, 2022
@steveisok
Copy link
Member

Disabling crypto tests on Android feels like a very heavy hammer. Especially since this is one of the areas where there's actually a lot of OS-specific code (so it's significantly more important to run the crypto tests on Android than it is to run, e.g. collections tests).

Are we running out of memory because there's a memory leak? If so, someone (with os-android experience) should investigate/fix it.

Is it just intermittent because it's down to test parallelization? Maybe disable test parallelization for the library (on Android).

I agree it's too heavy a hammer. I don't believe it's a memory leak. We've never been able to reproduce this outside of CI.

I think disabling test parallelization for this suite is something we should try. Disabling test randomization would be another thing to add as it may highlight where it fails more often.

@steveisok
Copy link
Member

On our team we do not have equipment or expertise to investigate device specific issues. Mono team handles this. @mdh1418 who on your team would look?

If we come up empty disabling parallel test execution, my team can take a closer look. Since we've never been able to reproduce locally, I suspect it'll be easier to look at the tests first and see if there's anything we can do to reduce contention. We would need @bartonjs to help us if we get to that point.

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…otnet#66833)

* Disabled System.Security.Cryptography.Tests.

* Applied @mdh1418 suggestion.

* Removed empty line.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2022
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.Security.Cryptography.Tests out of memory
5 participants