stryker-mutator / stryker-net

Mutation testing for .NET core and .NET framework!
https://stryker-mutator.io
Apache License 2.0
1.77k stars 184 forks source link

Suppress Assertion failed dialog when mutant uses invalid value verified by System.Diagnostics.Debug.Assert. #2375

Closed CzBuCHi closed 1 month ago

CzBuCHi commented 1 year ago

Is your feature request related to a problem? Please describe. Mutating code that uses System.Diagnostics.Debug.Assert( ... ) will show modal 'Assertion failed: abort / retry / ignore?' dialog
example: string mutation on string arg will trigger dialog for Debug.Assert(arg != "")

Describe the solution you'd like No 'Assertion failed' dialogs shown.

Describe alternatives you've considered

  1. remove all Debug.Assert calls from my code, but then i loose (my) sanity checks.
  2. replace all Debug.Assert(expr) with if(!expr) throw Exception(), but then i will have useless checks in release code.

My proposal solution

  1. Replace all Debug.Assert(expr) with if(!expr) throw new Exception() when mutating code. (This should also mark mutant as killed)
richardwerkman commented 1 year ago

I'd rather just remove the Debug.Assert statements as a whole. As the statement should not affect the code path in production it doesn't have to be mutated.

Also the execution could conflict with our testruns, as popups could show causing timeouts or worse. So you are right that we don't want to execute these statements. During testing I discovered that placing Debug.Assert(false) caused coverage analysis to fail. Which gave me a 0% score.

We should convert this to a bug with high priority.

CzBuCHi commented 1 year ago

My logic with replacing assert with if-throw was to kill mutants faster - say code after assertion is doing some long running operation before failing due invalid assert - mutant will be killed but it will survive longer, than need to ... but deleting asserts is valid resolution too

richardwerkman commented 1 year ago

The thing is that we alter behavior that way.

if(!expr) throw new Exception() would fail your test Debug.Assert(expr) would timeout your test, or make it succeed when ignored somehow

So yes, we would mark mutants as killed faster. But also incorrectly. We want all code to execute and then the test to fail. Not the code to fail itself.

dupdob commented 1 year ago

my two cents:

Definitely thinks this should be handled by test frameworks. That being said, Stryker could redirect Debug.Assert to throwing some exception to have a desirable result.

On the same topic, Stryker should not mutate Debug.Assert expressions by default. PS: S..t. Debug.Listeners is not available in Netcore, but one can use Trace.Listeners instead, which is available everywhere.

CzBuCHi commented 1 year ago

@richardwerkman my understaing of debug.assert is to do sanity check that is removed in release build and give programmer oportunity to attach debugger to that point and see call stack so he can fix whatever is causing that failure

so failed assert == failed test in my book (why have assert in first place if it has no effect on code?)

dupdob commented 1 year ago

I think we should share a clear specification for the expected behavior:

  1. Debug.Assert(....) statements should not be mutated (especially the predicate expression)
  2. Failed assertions must not display any UI
  3. Failed assertions should be assimilated to failed tests.

Is it correct?

CzBuCHi commented 1 year ago

@dupdob works for me ... maybe add another category to killed / survived / timeout - something like 'failed assertions' ?

dupdob commented 1 year ago

I would suggest a comment on how the mutation was killed (test or assertion), assuming we can forward this information through VsTest

dupdob commented 2 months ago

hi @Liam-Rougoor where do you stand regarding this feature ? if not, could you de-assign yourself as a signal for others to pick up.

In any case, thanks for the time you dedicated to this.