Repository navigation
Add additional System.Threading.Tasks tests - #2775
Conversation
I've ported the remainder of our Tasks tests from our internal sources. A lot of these are moved over unchanged (except for porting to xUnit). Some of the tests were not really designed to be run one after another, and left a lot of "orphaned" tasks doing a whole bunch of work even when the test case finished running. This was causing unpredictable performance, as some test would take a really long time in a "full run", but very little time if run individually. I've attempted to clean those cases up, so that each test case cleans up after the tasks it starts. I have also marked a decent chunk of these tests as OuterLoop because they tend to run for a very long time (some just by their nature, some probably can be optimized if we want to make them inner loop tests).
|
Fixes #702 |
|
@dotnet-bot test this, please |
There was a problem hiding this comment.
I was about to say that it's strange to have AggregateException tests here rather than in the contract from which AggregateException is exposed... and then I discovered that AggregateException is in fact exposed from System.Threading.Tasks. That's odd, but... oh well.
|
@mellinoe, thanks. There's an enormous amount of code here. At some point I'd like to review it all, but for now I just did a few spot checks to look for any glaring errors. I'm fine with you checking in once CI consistently passes. Note that the current failure that shows on Linux is also reproduceable on Windows... I was able to trigger it a handful of times by running that one test repeatedly (I made it a [Theory] that took a single int parameter and used a MemberData that yielded a thousand integers). |
|
Thanks for the first pass, Stephen. I will check out the flaky test tomorrow and see what is up. That was one of the last files I included so I didn't run those tests as many times as the others. I'll address the low-hanging fruit you mentioned, like the tests that can obviously be easily converted to [Theory]'s and the bad Assertion styles. I also plan on doing a more thorough cleanup / rewrite of some of these tests next week when I'm TestCop. There's a few things I want to at least accomplish at that time:
|
There was a problem hiding this comment.
I suppose we do want them now? 😉
There was a problem hiding this comment.
Good catch, added back and fixed up
Responding mostly to PR feedback and doing a bit of extra cleanup
There seems to be a potential product issue that needs investigating here.
|
Great that you'll be spending some more time on these next week. In the meantime, as an initial port, LGTM. |
Add additional System.Threading.Tasks tests
| Assert.ThrowsAsync<ArgumentNullException>( | ||
| () => future.ContinueWith<int>((Func<Task<int>, Object, int>)null, null, CancellationToken.None, TaskContinuationOptions.None, TaskScheduler.Default)); | ||
| Assert.ThrowsAsync<ArgumentNullException>( | ||
| () => future.ContinueWith<int>((t, s) => 2, null, CancellationToken.None, TaskContinuationOptions.None, null)); |
There was a problem hiding this comment.
Ran across these in master. None of these are awaited. Looks like a bug both now and at the time.
https://github.com/xunit/xunit/blob/2.1-beta3/src/xunit.assert/Asserts/ExceptionAsserts.cs#L50
There was a problem hiding this comment.
Ugh. Yeah, that's busted. I'll fix it. Thanks for pointing it out.
I've ported the remainder of our System.Threading.Tasks tests from our internal sources. A lot of these are moved over unchanged (except for porting to xUnit).
Some of the tests were not really designed to be run one after another, and left a lot of "orphaned" tasks doing a whole bunch of work even when the test case finished running. This was causing unpredictable performance, as some test would take a really long time in a "full run", but very little time if run individually, because there were tons of leftover tasks still running. I've attempted to clean those cases up, so that each test case cleans up after the tasks it starts. I have also marked a decent chunk of these tests as OuterLoop because they tend to run for a very long time (some just by their nature, some probably can be optimized if we want to make them inner loop tests).
@stephentoub