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

[wasm] Disable threading tests in System.Threading.Tasks.Extensions #38554

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jun 29, 2020

When disabling tests with async Task, about half the tests would be skipped. Instead of doing so, an assembly level skip will be added. This PR looks to make the System.Threading.Tasks.Extensions test suite pass on WebAssembly.
Tests run: 0, Errors: 0, Failures: 0, Skipped: 0. Time: 0.004518s

Contributes to #38422

@ghost
Copy link

ghost commented Jun 29, 2020

Tagging subscribers to this area: @tarekgh
Notify danmosemsft if you want to be subscribed.

@stephentoub
Copy link
Member

I have the same feedback on this that I've had on other such PRs. This is effectively disabling all of the meaningful tests for this library, and doing so in a noisy manner. If the problem is that "async Task" tests aren't supported, we should instead invest in adding support for that, and until it's supported, just disable the whole library's tests.

@steveisok
Copy link
Member

@stephentoub We'll disable tests for this whole library and others that fall into the same category.

Separately, it would be helpful to get your perspective on how we might fix async Task for (single threaded) wasm. My assessment has been to change the way the tests are written, which is a bit too costly. However, I am not an expert here and likely may be missing something.

@stephentoub
Copy link
Member

stephentoub commented Jun 29, 2020

Where is the code for the runner?

My assessment has been to change the way the tests are written

Change how? If you're testing asynchronous functionality, what is it you'd do differently?

@steveisok
Copy link
Member

The runner is here (@akoeplinger please fill in more detail if I miss anything)

https://github.com/dotnet/xharness/blob/master/src/Microsoft.DotNet.XHarness.TestRunners.Xunit/ThreadlessXunitTestRunner.cs

And following your post is something I've found worked (for some tests that were previously failing)

https://devblogs.microsoft.com/pfxteam/await-synchronizationcontext-and-console-apps/

@stephentoub
Copy link
Member

stephentoub commented Jun 29, 2020

And following your post is something I've found worked

Then why can't that approach be built into the runner?

Fundamentally, that's what needs to be done: rather than blocking waiting for the task, hook up a callback to the task and use whatever mechanisms are available to pump until that callback is invoked.

@mdh1418 mdh1418 force-pushed the mdhwang/skip_system_threading_tasks_extensions_tests branch from c4b97e0 to 469c133 Compare June 29, 2020 18:55
@mdh1418 mdh1418 merged commit 2573c96 into dotnet:master Jun 30, 2020
@mdh1418 mdh1418 deleted the mdhwang/skip_system_threading_tasks_extensions_tests branch June 30, 2020 12:38
@stephentoub
Copy link
Member

Separately, it would be helpful to get your perspective on how we might fix async Task for (single threaded) wasm

I worked with @akoeplinger on this today. He has a solution. Let's stop putting up PRs that disable every async Task test until that lands :-) Thanks!

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants