-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
CallBase() Solution fixes #8
Conversation
Hi srudin, I also thinked about whether to inherit IThrowsResult from ICallback or not. I decided not to do because I was unsure if this this is correct. Anyway I think the call "setup.Callback(() => currentStep++);" is now obsole because EndSetup already registers the "currentStep++". So you can reduce to Regards, |
Ok, changed this according to your comment. |
I think danielkzu has to decide, wether IThrowResult should derive from ICallback or not. So this is more a design issue. |
Thanks guys for taking the time. Not sure about allowing multiple Callback calls... that would require code changes too (that's what would happen if you made IThrowResult inherit from ICallback, I believe). Also, any chance you can Fetch + Rebase these commits? I've merged the fix to the Castle references, as well as the gitignore, so you should Skip those when rebasing as they shouldn't be needed anymore. Thanks! |
@daniel: I can't rebase because I'm not allowed to Force Push on github but I merged with the master. About the inheritance - I don't know the core of Moq so I really can't tell. All I know is my version with my changes can run the unit tests and is working with our large test project. But concerning implementation details - up to you... @lukas: I think you can't run the unit tests as long as the Castle references are incorrect - that's why I fixed them in my branch. |
You can always Force Push to your own fork. That's the beauty of Git ;). If you use TortoiseGit, you can do a Fetch (from upstream/master) from the /kzu Daniel Cazzulino On Mon, Jun 18, 2012 at 11:38 AM, Sandro <
|
Sorry - got myself irritated by the error message but I simply didn't check the Force Push box :-) Done now. |
Also, the multiple issues included in this pull request makes it harder to merge. i.e. the CallBase thing is totally unrelated with the Throws ... :( |
I agree that there are severall changes that are not directly related to each other. The reason for that is that it simply wasn't possible to compile the solution or run the unit tests without these changes. So I just commited what I had to adjust - quite franky I was a bit surprised about all the solution and project changes I had to make. I don't fully agree that the changes I made regarding CallBase() or Throws() are not related and I definitely don't think they are counter-intuitive. Fact is that I needed these changes in order to get some of our unit test previously mocked with TypeMock to run with Moq. I also believe that someone setting up a mock with SetupSequence() expects that something like .Returns(1).Return(2).Throws().Returns(3) is possible because when you setup a sequence you just want to define what happens on the first, second, third call no matter of what has happened before. As I said some of our tests expect this scenario and I can't see why this should be wrong in any way. After all I'm open to suggestions how this can be solved more properly. As I said before I have no insight in the Moq core but merely expanded the API to get access to functionality that was already there but not exposed. What we needed was Btw: Concerning b) I implemented what I needed for our tests but I still think not all functions are consequently available. |
By changing the interfaces defined in Language/Flow, you're not only Moq does not intend to replicate every other mocking framework features I think this feature requires a bit more thought. Maybe a new set of Flow That's why I insist on separating the CallBase one (which is isolated and Thanks! /kzu Daniel Cazzulino On Mon, Jun 18, 2012 at 6:44 PM, Sandro <
|
I extracted the CallBase changes and reduced the master branch to this change. I think you should be able to merge this now. Can I rely on you looking up the other changes now included in the dev branch and coming up with an alternative solution for them? Thanks! |
Fixes #20. Thanks for the clean fix :) |
Hello
I have implemented two features and fixed a couple of (minor) bugs:
First I added the option to be able to call the base (original) method for a specific setup – here’s a link where someone else has discussed this request: http://code.google.com/p/moq/issues/detail?id=293
Second I added the option that either Throw() or CallBase() is available within a setup sequence chain – again here’s a link where someone else has discussed this request: http://code.google.com/p/moq/issues/detail?id=319
Finally I adjusted the references in the UnitTest project to match those of the Moq project (I had compiler errors without that fix). I also adjusted the command line for merging the castle library into the moq library because the way it is specified now it will always compile for .NET runtime 4.0 even when the project is set to 3.5 and the 2.0 runtime should be used. And I added a .gitignore because I got tired of all the temp files being listed in my Commit dialogue.
I think the changeset is rather small and self explinatory but I admit that I’m not sure whether all my changes are implemented the best way (kind of tricky to keep the overview over all the interfaces) so maybe improvements are possible? Just get back to me if I can be of any help.
Thanks & kind regards
Sandro