Skip to content

Rework assert.multiple behavior under debugger to throw on each failure - #4749

Merged
OsirisTerje merged 3 commits into
nunit:mainfrom
MaceWindu:feature/4738-v2
Aug 4, 2024
Merged

OsirisTerje merged 3 commits into
nunit:mainfrom
MaceWindu:feature/4738-v2

Conversation

@MaceWindu

@MaceWindu MaceWindu commented Jul 9, 2024 •

Copy link
Copy Markdown
Contributor

Fixes #4738.

Instead of throwing on first failure, we will generate catched exception on each failure to allow user to debug it and still have multiple assert exception in the end

@OsirisTerje

Copy link
Copy Markdown
Member

@manfred-brands @stevenaw Can you have a look?

Comment thread src/NUnitFramework/framework/Api/FrameworkPackageSettings.cs
Comment thread src/NUnitFramework/framework/Assert.cs Outdated

@manfred-brands manfred-brands 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.

github doesn't allow me to comment on non-changed code.

@MaceWindu Should the ThrowOnEachFailureUnderDebugger property not be copied in the TestContext copy constructor?

Every child in a CompositeWorkItem aka a Fixture gets a new TestExecutionContext.
But the child will not have ThrowOnEachFailureUnderDebugger set, ever.

The only place where it is currently set is in the NUnitTestAssemblyRunner.

@OsirisTerje Not all the other properties are copied either.
TestCaseTimeout and StopOnError are, but Current(UI)Culture are not. Should they?

Comment thread src/NUnitFramework/framework/Assert.cs Outdated
Comment thread src/NUnitFramework/framework/Assert.cs Outdated
@OsirisTerje

Copy link
Copy Markdown
Member

@MaceWindu See comments from @manfred-brands - any upcoming action on those?

@manfred-brands Not all the other properties are copied either.
TestCaseTimeout and StopOnError are, but Current(UI)Culture are not. Should they?

Good question, meaning, IDK. I don't think they are being asked for, on the other side, it should not do any harm if they do.
I also noticed that over time we have added properties to TestContext from underlying classes, and they have not been documented. (Did that now)., So perhaps we should do a separate cleanup and documentation issue on that?

@MaceWindu

MaceWindu commented Jul 29, 2024 •

Copy link
Copy Markdown
Contributor Author

@OsirisTerje , I'will add copy of those properties. Just tested and can confirm options are not propagated to test context

@MaceWindu

MaceWindu commented Jul 29, 2024 •

Copy link
Copy Markdown
Contributor Author

BTW, CurrentCulture and CurrentUICulture not copied, as they are calculated properties over _sandboxedThreadState, which is copied already

@OsirisTerje OsirisTerje 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.

Ok, seems all is done here.

@OsirisTerje
OsirisTerje merged commit c7e915b into nunit:main Aug 4, 2024
@stevenaw

stevenaw commented Aug 4, 2024

Copy link
Copy Markdown
Member

Thanks @OsirisTerje
Had just been looking to approve this myself 👍

@MaceWindu
MaceWindu deleted the feature/4738-v2 branch August 5, 2024 07:07
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.

QoL suggestion: fail fast in Assert.Multiple under debugger

4 participants