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

Check why there's both MethodCall.ToString and MethodCall.Format, and whether we can drop one of them (ideally including MethodCall.SetFileInfo) #503

Closed
stakx opened this issue Oct 24, 2017 · 1 comment
Milestone

Comments

@stakx
Copy link
Contributor

stakx commented Oct 24, 2017

According to #500, .NET 4.7.1 has a regression that makes calls to new StackTrace() super costly. Moq only instantiates the StackTrace class for debugging purposes only, for the use in one of two formatting methods of MethodCall.

Why does MethodCall have two different formatting methods? Wouldn't a single one suffice? Ideally, we'd discover that the one using StackTrace information is the less frequently used one and can be replaced with the more commonly used one. Then StackTrace instantiation would become redundant and the .NET 4.7.1 performance issue evaporates.

@stakx
Copy link
Contributor Author

stakx commented Oct 24, 2017

ToString and Format are mostly redundant, having only one of them would be just fine.

MethodCall.ToString()

  • used for Verify() and VerifyAll() calls, i.e. with verification that targets possibly more than one setup
  • includes information about the code location where the setup was created

MethodCall.Format()

  • used for Verify(expression[, ...]) calls, i.e. with verification that targets a specific setup
  • also used when a maximum invocation count (setup via IOccurrence) is exceeded
  • does not include information about the code location where the setup was created

There's a argument to be made for keeping the information about the code location where a setup was created (derived from StackTrace), since with multi-setup verification, it helps pinpoint exactly which setups weren't matched. (Setting a fail message could also be used toward that end, but that's an explicit opt-in feature.)

The question remains whether this (useful) debugging information is worth the (unwelcome) potential performance penalty. Let's keep the feature for now. The current performance issue as linked in #500 will most likely be fixed in the .NET Framework so we shouldn't remove a useful feature too quickly.

P.S.: Starting with Moq 4.7.145, this diagnostic feature is now one you need to explictly opt-in; it's disabled by default. If you want source file information in verification error messages, enable it by setting mock[Repository].Switches |= Switches.CollectDiagnosticFileInfoForSetups.

@stakx stakx closed this as completed Nov 3, 2017
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

No branches or pull requests

1 participant