Add System.ComponentModel.EventBasedAsync tests #20931

hughbe · 2017-06-12T10:11:52Z

This increases CC to 100% in the library

hughbe · 2017-06-12T10:13:05Z

I may need some help here...

I get the following errors when I use RemoteInvoke, which don't occur without RemoteInvoke. They seem to be coming from the main method of RemoteExecutorConsoleApp...

  Discovering: System.ComponentModel.EventBasedAsync.Tests
  Discovered:  System.ComponentModel.EventBasedAsync.Tests
  Starting:    System.ComponentModel.EventBasedAsync.Tests
  Exception from RemoteExecutorConsoleApp(System.ComponentModel.EventBasedAsync.Tests, Version=4.1.1.0, Culture=neutral
  , PublicKeyToken=9d77cc7ad39b68eb, System.ComponentModel.Tests.AsyncOperationFinalizerTests+<>c__DisplayClass2_0, <Fi
  nalizer_OperationCompleted_CompletesOperation>b__0):
  Assembly: System.ComponentModel.EventBasedAsync.Tests, Version=4.1.1.0, Culture=neutral, PublicKeyToken=9d77cc7ad39b6
  8eb
  Type: System.ComponentModel.Tests.AsyncOperationFinalizerTests+<>c__DisplayClass2_0
  Method: Int32 <Finalizer_OperationCompleted_CompletesOperation>b__0()
  Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---
  > System.NullReferenceException: Object reference not set to an instance of an object.
     at System.ComponentModel.Tests.AsyncOperationFinalizerTests.<>c__DisplayClass2_0.<Finalizer_OperationCompleted_Com
  pletesOperation>b__0()
     --- End of inner exception stack trace ---
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
     at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
     at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
     at RemoteExecutorConsoleApp.Program.Main(String[] args)

  Unhandled Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invoc
  ation. ---> System.NullReferenceException: Object reference not set to an instance of an object.
     at System.ComponentModel.Tests.AsyncOperationFinalizerTests.<>c__DisplayClass2_0.<Finalizer_OperationCompleted_Com
  pletesOperation>b__0()
     --- End of inner exception stack trace ---
     at RemoteExecutorConsoleApp.Program.Main(String[] args)
     System.ComponentModel.Tests.AsyncOperationFinalizerTests.Finalizer_OperationCompleted_CompletesOperation [FAIL]
        Assert.Equal() Failure
        Expected: 42
        Actual:   -532462766
        Stack Trace:
  Exception from RemoteExecutorConsoleApp(System.ComponentModel.EventBasedAsync.Tests, Version=4.1.1.0, Culture=neutral
  , PublicKeyToken=9d77cc7ad39b68eb, System.ComponentModel.Tests.AsyncOperationFinalizerTests+<>c__DisplayClass1_0, <Fi
  nalizer_OperationCompleted_DoesNotCallOperationCompleted>b__0):
  Assembly: System.ComponentModel.EventBasedAsync.Tests, Version=4.1.1.0, Culture=neutral, PublicKeyToken=9d77cc7ad39b6
  8eb
  Type: System.ComponentModel.Tests.AsyncOperationFinalizerTests+<>c__DisplayClass1_0
  Method: Int32 <Finalizer_OperationCompleted_DoesNotCallOperationCompleted>b__0()
              at System.Diagnostics.RemoteExecutorTestBase.RemoteInvokeHandle.Dispose()
              at System.ComponentModel.Tests.AsyncOperationFinalizerTests.Finalizer_OperationCompleted_CompletesOperati
  on()
  Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---
  > System.NullReferenceException: Object reference not set to an instance of an object.
     at System.ComponentModel.Tests.AsyncOperationFinalizerTests.<>c__DisplayClass1_0.<Finalizer_OperationCompleted_Doe
  sNotCallOperationCompleted>b__0()
     --- End of inner exception stack trace ---
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
     at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
     at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
     at RemoteExecutorConsoleApp.Program.Main(String[] args)

  Unhandled Exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invoc
  ation. ---> System.NullReferenceException: Object reference not set to an instance of an object.
     at System.ComponentModel.Tests.AsyncOperationFinalizerTests.<>c__DisplayClass1_0.<Finalizer_OperationCompleted_Doe
  sNotCallOperationCompleted>b__0()
     --- End of inner exception stack trace ---
     at RemoteExecutorConsoleApp.Program.Main(String[] args)
     System.ComponentModel.Tests.AsyncOperationFinalizerTests.Finalizer_OperationCompleted_DoesNotCallOperationComplete
  d [FAIL]
        Assert.Equal() Failure
        Expected: 42
        Actual:   -532462766
        Stack Trace:
  Finished:    System.ComponentModel.EventBasedAsync.Tests

If anyone knows what I've done wrong, I'd appreciate some assistance

stephentoub · 2017-06-12T11:52:34Z

I get the following errors when I use RemoteInvoke, which don't occur without RemoteInvoke

The lambda you give to RemoteInvoke must not close over anything. We only marshal the name of the lambda method plus any arguments explicitly passed as arguments to RemoteInvoke; when the display class is instantiated in the remote process, the lifted fields accessed by the display class would be null.

hughbe · 2017-06-12T12:12:46Z

That'd be it, thanks!

stephentoub · 2017-06-12T18:14:04Z

src/System.ComponentModel.EventBasedAsync/tests/AsyncOperationFinalizerTests.cs

+        {
+            MethodInfo finalizer = typeof(AsyncOperation).GetMethod("Finalize", BindingFlags.NonPublic | BindingFlags.Instance);
+            Assert.NotNull(finalizer);
+        }


It'd be better to actually test the functionality of the finalizer rather than testing for its presence via reflection and invoking it explicitly as is done below. You could do this by creating your own sync ctx that the finalizer will call back to.

Correct me if I'm wrong, the tests do exactly that. They use the following sync context that the finalizer does indeed call back to when we manually invoke it. This test you commented on is just a sanity checker.

public class OperationCompletedTracker : SynchronizationContext { public int OperationCompletedCounter { get; set; } public override void OperationCompleted() => OperationCompletedCounter++; }

In terms of guaranteeing that the finalizer is invoked without reflection, I don't know what the best way to do that is

They use the following sync context that the finalizer does indeed call back to when we manually invoke it.

But we shouldn't manually invoke it via reflection. You instead need to ensure that the object is collectable (e.g. ctor it in a helper method to ensure the JIT doesn't artificially extend its lifetime), and then force collection with GC.Collect and wait for finalizers to run with GC.WaitForPendingFinalizers.

stephentoub · 2017-06-12T18:15:28Z

src/System.ComponentModel.EventBasedAsync/tests/BackgroundWorkerTests.cs

+                Assert.Null(e.Result);
+                Assert.False(backgroundWorker.IsBusy);
+            };
+            backgroundWorker.RunWorkerAsync();


This isn't going to play particularly nicely with xunit. Any failures are going to happen asynchronously, maybe after the test has already completed, and in fact the process may shut down before it gets a chance to run.

Interesting. I've updated the test to handle this fact, and also to make sure that the completed handler was called

safern

Other than Stephen's comments LGTM.

safern · 2017-06-14T01:11:53Z

The tests you're adding use RemoteExecutor and that will break this tests for UWP since RemoteExecutor is broken. There is a PR open to fix that, so I would like this PR to be merged after the other one since we are trying to get our test failures in UWP to 0 so that we can enable a CI leg.

#21014 this is the PR that we should wait for.

hughbe · 2017-06-14T01:19:07Z

Sure thing @safern

hughbe · 2017-06-16T15:05:12Z

Test Innerloop OSX10.12 Release x64 Build and Test

hughbe · 2017-06-17T01:26:49Z

Test Innerloop OSX10.12 Release x64 Build and Test

hughbe · 2017-06-17T03:49:57Z

I've removed the reflection - I think the blocker for this PR for RemoteExecutorTestBase is no more, right?

safern · 2017-06-17T08:28:30Z

I've removed the reflection - I think the blocker for this PR for RemoteExecutorTestBase is no more, right?

I'll pull your changes and confirm locally that the tests don't cause any regression for uap, so that we don't go backwards in the progress we've made.

hughbe · 2017-06-19T07:00:24Z

@safern I know it was the weekend, but let me know how it goes :)

safern · 2017-06-19T21:06:30Z

@safern I know it was the weekend, but let me know how it goes :)

Taking a look now. I'll let you know :)

safern · 2017-06-19T23:49:24Z

They pass in Uap so I will merge :) Thanks @hughbe

* Add System.ComponentModel.EventBasedAsync tests Commit migrated from dotnet/corefx@e4a633a

dnfclas added the cla-already-signed label Jun 12, 2017

stephentoub reviewed Jun 12, 2017

View reviewed changes

karelz added the area-System.ComponentModel label Jun 12, 2017

karelz assigned hughbe, AlexGhiondea and safern Jun 12, 2017

karelz modified the milestone: 2.1.0 Jun 12, 2017

safern approved these changes Jun 14, 2017

View reviewed changes

safern added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 14, 2017

hughbe added 4 commits June 16, 2017 18:39

Add System.ComponentModel.EventBasedAsync tests

145afbf

Fix RemoteInvoke test failures

5c765f0

Fix test failures potentially happpening after the test has finished

c2a746b

Address PR feedback

f142bc8

safern merged commit e4a633a into dotnet:master Jun 19, 2017

safern removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 19, 2017

hughbe deleted the eventbasedasync-fixes branch June 19, 2017 23:52

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add System.ComponentModel.EventBasedAsync tests (dotnet/corefx#20931)

f4ce963

* Add System.ComponentModel.EventBasedAsync tests Commit migrated from dotnet/corefx@e4a633a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add System.ComponentModel.EventBasedAsync tests #20931

Add System.ComponentModel.EventBasedAsync tests #20931

hughbe commented Jun 12, 2017

hughbe commented Jun 12, 2017

stephentoub commented Jun 12, 2017

hughbe commented Jun 12, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

stephentoub Jun 13, 2017

stephentoub Jun 12, 2017

hughbe Jun 13, 2017

safern left a comment

safern commented Jun 14, 2017

hughbe commented Jun 14, 2017

hughbe commented Jun 16, 2017

hughbe commented Jun 17, 2017

hughbe commented Jun 17, 2017

safern commented Jun 17, 2017 •

edited

Loading

hughbe commented Jun 19, 2017

safern commented Jun 19, 2017

safern commented Jun 19, 2017

Add System.ComponentModel.EventBasedAsync tests #20931

Add System.ComponentModel.EventBasedAsync tests #20931

Conversation

hughbe commented Jun 12, 2017

hughbe commented Jun 12, 2017

stephentoub commented Jun 12, 2017

hughbe commented Jun 12, 2017

stephentoub Jun 12, 2017

Choose a reason for hiding this comment

hughbe Jun 13, 2017

Choose a reason for hiding this comment

stephentoub Jun 13, 2017

Choose a reason for hiding this comment

stephentoub Jun 12, 2017

Choose a reason for hiding this comment

hughbe Jun 13, 2017

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

safern commented Jun 14, 2017

hughbe commented Jun 14, 2017

hughbe commented Jun 16, 2017

hughbe commented Jun 17, 2017

hughbe commented Jun 17, 2017

safern commented Jun 17, 2017 • edited Loading

hughbe commented Jun 19, 2017

safern commented Jun 19, 2017

safern commented Jun 19, 2017

safern commented Jun 17, 2017 •

edited

Loading