Skip to content

[wasm] Disable threading tests in System.Linq.Parallel#38382

Merged
stephentoub merged 1 commit intodotnet:masterfrom
akoeplinger:wasm-linq-parallel
Jun 25, 2020
Merged

[wasm] Disable threading tests in System.Linq.Parallel#38382
stephentoub merged 1 commit intodotnet:masterfrom
akoeplinger:wasm-linq-parallel

Conversation

@akoeplinger
Copy link
Member

This allows the test suite to pass on WASM: Tests run: 27708, Errors: 0, Failures: 0, Skipped: 107. Time: 159.380298s

This allows the test suite to pass on WASM: `Tests run: 27708, Errors: 0, Failures: 0, Skipped: 107. Time: 159.380298s`
@akoeplinger akoeplinger requested review from mdh1418 and steveisok June 25, 2020 12:05
@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

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.

Looks good to me!

@stephentoub
Copy link
Member

Did you run outerloop?

@stephentoub
Copy link
Member

This allows the test suite to pass on WASM

What's the behavior of the skipped tests when they actually run? A hang?

@akoeplinger
Copy link
Member Author

Did you run outerloop?

No, we haven't run the outerloop tests yet on any of the mobile platforms. That's a future step :)

What's the behavior of the skipped tests when they actually run? A hang?

The OrderBy_ThreadedDeadlock_SingleException hung indeed, the rest throw the usual System.Threading.SynchronizationLockException: Cannot wait on events on this runtime.

{
if (partitions > 1 && !PlatformDetection.IsThreadingSupported)
{
throw new SkipTestException(nameof(PlatformDetection.IsThreadingSupported));
Copy link
Member

@stephentoub stephentoub Jun 25, 2020

Choose a reason for hiding this comment

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

This pattern seems pretty common across the various PRs being opened in this realm... maybe we should add a:

public static void ThrowSkipExceptionIfThreadingNotSupported();

to PlatformDetection. Not a big deal, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now it's only four cases across all the PRs we did so I feel like we don't need it yet.


// Heavily exercises OrderBy, but only throws one user delegate exception to simulate an occasional failure.
[Theory]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
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 surprised so few tests needed to be disabled. I guess we're mostly saved by PLINQ's fall back to single-threaded execution when ProcessorCount == 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was surprised too and came to the same conclusion.

@stephentoub stephentoub merged commit f4ed217 into dotnet:master Jun 25, 2020
@akoeplinger akoeplinger deleted the wasm-linq-parallel branch June 25, 2020 13:54
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
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.

5 participants