Skip to content

Redundant AggregateException removal#1038

Merged
jnyrup merged 3 commits intofluentassertions:masterfrom
davidomid:AggregateException-removal
May 4, 2019
Merged

Redundant AggregateException removal#1038
jnyrup merged 3 commits intofluentassertions:masterfrom
davidomid:AggregateException-removal

Conversation

@davidomid
Copy link
Copy Markdown
Contributor

Resolves issue #1037

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented May 3, 2019

I agree that it is better to .GetAwaiter().GetResult() as that should remove the need to unwrap AggregateExceptions, as we then know they are not added.

I'm a bit suspicious/worried that we need to alter a test, that means the behavior has changed.
We seem to be inconsistent in how we have been unwrapping AggregateExceptions :(.

NotThrow and NotThrow<TException> call GetFirstNonAggregateException, which unwraps all AggregateExceptions.

NotThrowAfter and Throw<TException> call InvokeSubjectWithInterception, which only unwraps an AggregateException if its InnerException is also an AggregateException.

@dennisdoomen
Copy link
Copy Markdown
Member

I agree with your concern. In general, I think we should unwrap an AggregateException if it is FluentAssertions that causes it to be inserted. Only if invoking a Task directly would also raise an AggregateException, we should expose it. Throw<TException>() is special here. If you call Should().Throw<InvalidOperationException>() and the calling code raises this specific exception wrapped in an AggregateException, I believe it should succeed. This is the way it is currently implemented and documented. The same (should) apply to ThrowExactly, ThrowAfter, etc.

@dennisdoomen dennisdoomen requested a review from jnyrup May 4, 2019 10:09
@davidomid
Copy link
Copy Markdown
Contributor Author

@jnyrup @dennisdoomen

I've added another passing unit test to make sure we can check for an exception type when an AggregateException contains an instance of it:

        [Fact]
        public void When_async_method_throws_aggregate_exception_containing_expected_exception_it_should_succeed()
        {
            //-----------------------------------------------------------------------------------------------------------
            // Arrange
            //-----------------------------------------------------------------------------------------------------------
            Func<Task> task = async () =>
            {
                await Task.Delay(100);
                throw new AggregateException(new InvalidOperationException());
            };

            //-----------------------------------------------------------------------------------------------------------
            // Act
            //-----------------------------------------------------------------------------------------------------------
            Action action = () => task
                .Should().Throw<InvalidOperationException>();

            //-----------------------------------------------------------------------------------------------------------
            // Assert
            //-----------------------------------------------------------------------------------------------------------
            action.Should().NotThrow();
        }

The latest build fails due to one of those flaky tests again.

Aside from adding this test case, are there any other changes I should make at this stage?

Thanks very much

Copy link
Copy Markdown
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

Aha, I wasn't aware of the AggregateException unwrapping feature of FA.
Then it looks good to me 👍

It also inspired me to find some cases, where we wasn't consistent for NotThrow<TException> (PR coming up af this is merged)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants