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] Enable System.Threading.Tasks.Extensions tests #38815

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jul 6, 2020

Contributes to #38422
Tests run: 184, Errors: 0, Failures: 0, Skipped: 1. Time: 0.350004s

@ghost
Copy link

ghost commented Jul 6, 2020

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

@stephentoub
Copy link
Member

The following 9 tests all failed with System.PlatformNotSupportedException : Cannot wait on monitors on this runtime.

Most of these can easily be tweaked to avoid needing that.

@steveisok
Copy link
Member

@stephentoub What would you suggest?

@stephentoub stephentoub force-pushed the mdhwang/properly_skip_system_threading_tasks_extensions_tests branch from 5d19548 to 13fe8ee Compare July 7, 2020 13:54
@stephentoub
Copy link
Member

@stephentoub What would you suggest?

Like this:
13fe8ee

@stephentoub
Copy link
Member

This test suite still has a bunch of conditional checks for IsThreadingSupported... are all of those still needed? Or are they leftover from before async Task tests worked? If the latter, can we clean that up here and anywhere else we may have created debt? Thanks.

@mdh1418
Copy link
Member Author

mdh1418 commented Jul 7, 2020

Some new tests were failing locally for me, so I changed them to ConditionalFact

  • System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.OnCompleted_ContinuationAlwaysInvokedAsynchronously
  • System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.SetResult_RunContinuationsAsynchronously_ContinuationInvokedAccordingly

Both of these had tp callback threw a Xunit.Sdk.NotEqualException.

The other instances of IsThreadingSupported are necessary because the tests otherwise hang. I reran for each of the 23 skipped tests, trying with [Fact] or [Theory] instead, and ran for each of the test cases.

[SKIP] System.Threading.Tasks.Tests.AsyncValueTaskMethodBuilderTests.PoolingAsyncValueTasksBuilder_ObjectsPooled
[SKIP] System.Threading.Tasks.Tests.AsyncValueTaskMethodBuilderTests.AwaitTasksAndValueTasks_InTaskAndValueTaskMethods
[SKIP] System.Threading.Tasks.Tests.AsyncValueTaskMethodBuilderTests.Generic_ConcurrentBuilders_WorkCorrectly
[SKIP] System.Threading.Tasks.Tests.AsyncValueTaskMethodBuilderTests.NonGeneric_ConcurrentBuilders_WorkCorrectly
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.SetException_AfterOnCompleted_ResultAvailableAsynchronously
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.AsyncEnumerable_Success
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.SetResult_AfterOnCompleted_ResultAvailableAsynchronously
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.ReuseInstanceWithResets_Success
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.OnCompleted_ContinuationAlwaysInvokedAsynchronously
[SKIP] System.Threading.Tasks.Sources.Tests.ManualResetValueTaskSourceTests.SetResult_RunContinuationsAsynchronously_ContinuationInvokedAccordingly
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_CreateFromValueTaskSource_Canceled(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_CreateFromValueTaskSource_Success(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_CreateFromValueTaskSource_Faulted(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_CreateFromTask_Await_Normal
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_CreateFromValueTaskSource_Await_Normal
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.CreateFromValueTaskSource_Await_Normal
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_Awaiter_ContinuesOnCapturedContext
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.NonGeneric_CreateFromTask_Await_Normal
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.NonGeneric_ConfiguredAwaiter_ContinuesOnCapturedContext
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.NonGeneric_CreateFromValueTaskSource_Canceled(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.NonGeneric_CreateFromValueTaskSource_Faulted(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.NonGeneric_CreateFromValueTaskSource_Success(sync: False)
[SKIP] System.Threading.Tasks.Tests.ValueTaskTests.Generic_ConfiguredAwaiter_ContinuesOnCapturedContext

If there were other instances of IsThreadingSupported under this suite not in this list then I missed them, I take it they should've been skipped as well.

@akoeplinger
Copy link
Member

akoeplinger commented Jul 7, 2020

Both of these had tp callback threw a Xunit.Sdk.NotEqualException.

That's expected since the test does Assert.NotEqual(threadId, Environment.CurrentManagedThreadId); to verify there's a different thread, which won't be true in a single-thread environment.

Side note: I wonder whether this should've actually triggered the AppDomain.UnhandledException instead of printing this message:

if (exc) {
char *type_name = mono_type_get_full_name (mono_object_class (exc));
printf ("tp callback threw a %s\n", type_name);
g_free (type_name);

@stephentoub
Copy link
Member

stephentoub commented Jul 7, 2020

That's expected since the test does Assert.NotEqual(threadId, Environment.CurrentManagedThreadId); to verify there's a different thread, which won't be true in a single-thread environment.

The goal of the tests isn't to validate it's a different thread, just that the continuation is invoked asynchronously rather than synchronously. We can validate that in other ways. For example, we can store a value into an ThreadLocal<T> and set some sentinel value in it, then in the callback validate that the ThreadLocal<T> does or does not contain the desired value, and then remove the value from the ThreadLocal<T> after the line that does the validation, e.g.

var tl = new ThreadLocal<T> { Value = 42 };
InvokeAsynchronously(() => Assert.NotEqual(42, tl.Value));
tl.Value = 0;

In general I'd like disabling of such tests to be a last resort.

@@ -399,7 +399,7 @@ async ValueTask<int> ValueTaskReturningAsyncMethod(int result)
}
}

[Fact]
[ConditionalFact(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.

If the test is hanging, shouldn't that be an ActiveIssue?

Copy link
Member

@akoeplinger akoeplinger Jul 7, 2020

Choose a reason for hiding this comment

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

I've looked at it and it runs into an infinite loop in the threadpool pump loop in xharness as soon as we hit the "Incomplete" tests. I didn't have time to investigate more deeply but I agree, we should file an ActiveIssue for these cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created an ActiveIssue here #38931

@akoeplinger
Copy link
Member

The goal of the tests isn't to validate it's a different thread, just that the continuation is invoked asynchronously rather than synchronously

Yeah I meant that was what the test is doing right now, we can obviously rewrite it :)

@mdh1418 mdh1418 force-pushed the mdhwang/properly_skip_system_threading_tasks_extensions_tests branch from 9e20e84 to df224a9 Compare July 8, 2020 15:02
@safern safern mentioned this pull request Jul 8, 2020
@mdh1418
Copy link
Member Author

mdh1418 commented Jul 9, 2020

The failing lane is in Linux x64, which is unrelated to changes in System.Threading.Tasks.Extensions for wasm.

@mdh1418 mdh1418 merged commit bb204f8 into dotnet:master Jul 9, 2020
@mdh1418 mdh1418 deleted the mdhwang/properly_skip_system_threading_tasks_extensions_tests branch July 9, 2020 18:43
@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