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

Expose Moq.Times' Verify and Kind #970

Closed
dmcbride opened this issue Feb 7, 2020 · 6 comments · Fixed by #975
Closed

Expose Moq.Times' Verify and Kind #970

dmcbride opened this issue Feb 7, 2020 · 6 comments · Fixed by #975
Milestone

Comments

@dmcbride
Copy link

dmcbride commented Feb 7, 2020

We would like to write some additional moq-like tests specific to our environment where we are counting items that must fit in a range the same way that Times already does. Now, we could basically reproduce the Moq.Times struct in our own code, but that seems awfully wasteful when all we would need from Moq is to expose the Verify method and the Kind of the object, and we could easily reuse the struct for any other counting purpose.

We have to reproduce the GetExceptionMessage method as the message itself would be dramatically different for our use case, but there's nothing that can be done about that. However, we cannot reproduce it without access to Kind as we wouldn't know which message to use to give the developer the most useful exception message.

@stakx
Copy link
Contributor

stakx commented Feb 7, 2020

However, we cannot reproduce it without access to Kind as we wouldn't know which message to use to give the developer the most useful exception message.

Kind is an implementation detail, it shouldn't be exposed.

Why not deconstruct a Times instance into a from..to range and infer the exception message from that? While this would mean that you cannot distinguish between (for instance) Once and Exactly(1), this might not actually matter to users.

@dmcbride
Copy link
Author

dmcbride commented Feb 7, 2020

The deconstruction is what we're going to have to do for now, but that means we're duplicating the code already in Verify (ok, it's small, not the end of the world), and the logic for determining the Kind would be a heuristic rather than exact (e.g., the difference between Times.Range(1, 3, Range.Inclusive) and Times.Range(0,4,Range.Exclusive)). And the exact message can be helpful in determining which piece of code is calling Times - if I'm looking for the code based on a test failure message of "count expected between 1 and 3, but got 5" then I'm looking for the 1 and 3 in my test code, not the 0 and 4 with a Range.Exclusive.

@stakx
Copy link
Contributor

stakx commented Mar 2, 2020

I understand how exposing Times.kind would be helpful in your case. But I still think it'd be a mistake for Moq. Right now, we can add new factory methods to Times if we want. Once we expose kind as an enum, we'll cement the available options, as adding additional enum members can break user code (e.g. switch blocks all of a sudden falling through to the default case where they previously didn't).

It would be safer to instead expose some IEqualityComparer<Times> that can tell the difference between equivalent instances such as those you mentioned. Or, somewhat closer to exposing kind, exposing a MethodInfo Constructor property that points at the factory method used to create a particular instance.

Those would however be somewhat strange (highly specific) things to add to Moq's API that most people will probably never use.

@stakx
Copy link
Contributor

stakx commented Mar 3, 2020

I checked with Moq 5's Times implementation, which is very similar to Moq 4's, but also has a .ToString() implementation, which we are currently missing here.

So here's another idea @dmcbride: What if we added a .ToString() implementation to Moq 4 that produces the following results (e.g.):

  • Times.Once.ToString()"Once"
  • Times.Exactly(1).ToString()"Exactly(1)"
  • Times.Between(1, 4, Range.Exclusive)"Between(1, 4, Exclusive)"
  • Times.Between(1, 4, Range.Inclusive)"Between(1, 4, Inclusive)"
  • etc.

That is, the string representation would be very much akin to the code used to build the instance, allowing you to search for the static factory method names to figure out what kind a Times instance is.

It certainly isn't the most ideal solution for your exact problem, but should work well for the general case.

I'm also fine with exposing the .Verify(int callCount) method under a different signature (.Validate(int count)), since Moq 5 already does the same thing.

@dmcbride
Copy link
Author

dmcbride commented Mar 9, 2020

The resultant message would be less than perfect English, but since it's not going to users, only developers, I think that would work very well. We'd end up with code that calls Validate, and if that returns false, throws an exception with a message like $"Expected {kind}, got {count}" - and that should make it really easy to find and recognise the code. (Yes, the stack trace already points the line number, but with this, the brain will be primed to recognise it.)

@stakx
Copy link
Contributor

stakx commented Mar 9, 2020

Consider it done @dmcbride .

The resultant message would be less than perfect English [...]

An unparameterized ToString method doesn't have enough context anyway to guarantee that you can embed the returned string in any English sentence. (That's the reason why the internal method Times.GetExceptionMessage loads resource strings containing full sentences; one of them uses a different sentence structure.) So I believe it's perfectly sensible that ToString produces something "technical".

(You could of course parse the returned string into whatever English representation you like — as long as we don't change the formatting of Times, that should work just fine. (You could validate your assumptions regarding the values returned by Times.ToString using a set of unit tests, so you'll be notified if and when things change.))

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

Successfully merging a pull request may close this issue.

2 participants