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

Added test case to test the Exception aggregation #142

Closed
wants to merge 9 commits into from
Closed

Added test case to test the Exception aggregation #142

wants to merge 9 commits into from

Conversation

RyanDawkins
Copy link

Added test case to test the Exception aggregation

Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

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

This is great.

@drub0y has a major set of changes to the TestAdapter coming tonight / tomorrow that fixes problems in how Tasks are handled in exceptional cases. Specifically he's unwinding Tasks and handling and a few corner cases that I'm not sure I understand.

If you don't mind, let's wait on this PR until that one is in, then we can bring the revised test over.

@drub0y
Copy link
Contributor

drub0y commented Feb 17, 2018

Those changes just went in and added one more test to the suite to cover the scenario where the exception happens in OnReceive. It also changes the exception handling so that the underlying exception propagates more "naturally" through the task chain as a AggregateException. Glancing at the test you added here it should work exactly the same way since you're checking the inner exception's type. So if you rebate against my commit and rerun it should "just work". 😊

@cleemullins
Copy link
Contributor

cleemullins commented Feb 17, 2018 via email

Pass exceptions on Faulted Tasks as inner Exceptions.
@cleemullins
Copy link
Contributor

cleemullins commented Feb 17, 2018 via email

@RyanDawkins
Copy link
Author

I'm about to pull in the changes if you pulled stuff into your branch.

@RyanDawkins
Copy link
Author

RyanDawkins commented Feb 17, 2018

@cleemullins I can close this PR and open another for master? I noticed you already merged your branch in.

Edit: I'm going to close it anyways because now I'm just updating your branch with master.

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