Skip to content

Handle Exceptions thrown during discovery, such as in Custom Attributes - #4104

Merged
stevenaw merged 1 commit into
nunit:v3.13-devfrom
manfred-brands:issue/4096
Jun 11, 2022
Merged

stevenaw merged 1 commit into
nunit:v3.13-devfrom
manfred-brands:issue/4096

Conversation

@manfred-brands

@manfred-brands manfred-brands commented Apr 23, 2022

Copy link
Copy Markdown
Member

Fixes #4096
Fixes #4107

Tests with Exceptions are Marked As Invalid and show up as an Error.
Fixtures with Exceptions are non-runnable and don't show up with dotnet test.

One alternative option would be to make a special fixture with one runable method that fails.

@stevenaw stevenaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @manfred-brands ! Great that this could be fixed so simply.
Two minor nitpicks/ suggestions. I also would appreciate a second review from someone else

Comment thread src/NUnitFramework/framework/Internal/Builders/DefaultTestCaseBuilder.cs Outdated
Comment thread src/NUnitFramework/testdata/AttributeThrowingExceptionFixture.cs Outdated
@manfred-brands

Copy link
Copy Markdown
Member Author

Thanks @stevenaw, changes made.

@stevenaw

Copy link
Copy Markdown
Member

Very cool, thanks @manfred-brands ! Looks great to me 👍

@manfred-brands

Copy link
Copy Markdown
Member Author

Is there anything blocking this from being merged in?

@stevenaw

stevenaw commented Jun 4, 2022

Copy link
Copy Markdown
Member

@manfred-brands I'd been hoping to get a second pair of eyes on it since this is a part of the codebase I'm less familiar with, but perhaps it could also be merged as-is.
@mikkelbu @rprouse are either of you available to do a review?

@mikkelbu

mikkelbu commented Jun 6, 2022

Copy link
Copy Markdown
Member

I'm sorry. This PR got lost in too many GitHub notifications. I'll take a look at this today or tomorrow

@rprouse rprouse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the delay. This looks good. For anyone else reviewing, ignore whitespace, it is much easier to read.

@mikkelbu mikkelbu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I've only some very minor comments 👍

Comment thread src/NUnitFramework/framework/Internal/Builders/DefaultTestCaseBuilder.cs Outdated
Comment thread src/NUnitFramework/framework/Internal/Builders/NUnitTestCaseBuilder.cs Outdated
Comment thread src/NUnitFramework/framework/Internal/Builders/DefaultTestCaseBuilder.cs Outdated
@manfred-brands
manfred-brands force-pushed the issue/4096 branch 2 times, most recently from 3a231b5 to 1e09c55 Compare June 11, 2022 02:20
@manfred-brands

Copy link
Copy Markdown
Member Author

Cherry pick into master depends on #4053

@manfred-brands

Copy link
Copy Markdown
Member Author

This is ready for merge into v3.13-dev now.

@stevenaw

Copy link
Copy Markdown
Member

Thanks @manfred-brands ! LGTM 🎉

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.

5 participants