try fix some deadlocks #1527
try fix some deadlocks #1527
Conversation
|
FYI this will help to contribute to #1284. |
|
@Evangelink is there a way to update the api test result to match the api changes? |
|
@Evangelink ok this is ready for a review |
There was a problem hiding this comment.
First pass LGTM.
I will need to review the changes to public API to see if it's used on GitHub, and I also want to have a more in-depth look at some of the semaphore changes.
Side question, do you have any example of scenario where you hit the deadlock? Just trying to see if we could improve our tests (which isn't great at the moment).
a consumer of Verify.MsTest hit it here VerifyTests/Verify#751 and my guess was it is related to MSTests use of getawaiter, but i cant be sure. So no i dont think i could produce a test which would reliably reproduce a deadlock |
| @@ -41,8 +42,8 @@ public TestMethodAttribute(string? displayName) | |||
| /// <param name="testMethod">The test method to execute.</param> | |||
| /// <returns>An array of TestResult objects that represent the outcome(s) of the test.</returns> | |||
| /// <remarks>Extensions can override this method to customize running a TestMethod.</remarks> | |||
| public virtual TestResult[] Execute(ITestMethod testMethod) | |||
| public virtual async Task<TestResult[]> Execute(ITestMethod testMethod) | |||
There was a problem hiding this comment.
I need to think about this one, as far as I can see it's quite often overridden. I am not sure if that's better to break or offer the two methods Execute and ExecuteAsync and obsolete the former.
| @@ -57,7 +58,7 @@ public interface ITestMethod | |||
| /// <remarks> | |||
| /// This call handles asynchronous test methods as well. | |||
| /// </remarks> | |||
| TestResult Invoke(object[]? arguments); | |||
| Task<TestResult> Invoke(object[]? arguments); | |||
There was a problem hiding this comment.
Same remark here, it's also used in a few places.
| @@ -21,7 +23,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; | |||
| /// </summary> | |||
| public class TestClassInfo | |||
There was a problem hiding this comment.
As far as I can see on GitHub and internal repo this is not used outside of MSTest so changes to public API are fine (I will do a follow-up PR changing accessibility where possible to reduce un-needed/wanted public surface).
| @@ -320,36 +313,33 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC | |||
|
|
|||
| for (int i = 0; i < parallelWorkers; i++) | |||
| { | |||
| tasks.Add(_taskFactory(() => | |||
There was a problem hiding this comment.
I need to look more into this part because it's changing the logic (I am not sure if that's good or not).
There was a problem hiding this comment.
yeah given i cant compile (and hence cant run locally) i was guessing as to the correct new code.
happy to take direction from you
There was a problem hiding this comment.
Probably better to keep the old behaviour in here, we are missing tests to be 100% sure of what was the real intent here.
Co-authored-by: Amaury Levé <amaury.leve@gmail.com>
|
rebased |
|
@SimonCropp I am curious as to what issues you are facing with building, would you mind sharing the errors? |
|
@SimonCropp I am starting to think it's probably better to go full breaking change for v4. It's likely to break users extending MSTest but it feels like lots of extra work and increased likelihood of deadlock to keep the |
help to contribute to #1284.