Skip to content

Ensure that FailureSite.Child is used where appropriate. #2671

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

Merged
merged 2 commits into from
Jan 23, 2018

Conversation

CharliePoole
Copy link
Member

@CharliePoole CharliePoole commented Jan 20, 2018

Fixes #2670

This PR makes sure that a suite with a Warning result due to a child test is always given FailureSite.Child just as is done for Failed results. By doing this, we allow runners to display the warning where it occurs without needing to parser the wording of the message.

jnm2
jnm2 previously approved these changes Jan 20, 2018
mikkelbu
mikkelbu previously approved these changes Jan 20, 2018
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Only a minor comment

@@ -134,6 +134,11 @@ public ResultState(TestStatus status, string label, FailureSite site)
/// </summary>
public readonly static ResultState ChildFailure = ResultState.Failure.WithSite(FailureSite.Child);

/// <summary>
/// A suite failed because one or more child tests failed or had errors
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we change this comment, so that it mentions warning? E.g.
/// A suite failed because one or more child tests had warnings
Currently, it is just the same comment as for ChildFailure

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@CharliePoole CharliePoole dismissed stale reviews from mikkelbu and jnm2 via d593fc1 January 20, 2018 21:19
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants