The mocking bird

[gildub]

Can we please remove mock?

There are several reasons to remove the Mock class or at least move it away into unit tests. Also, I believe I'm not the only one thinking in such way about the mock, so it seems to be the time to discus it.

Looking back, some time ago, I was thinking having unit testing capability close to the source was a good idea, meanwhile experience proved otherwise.

The mock class is more a burden than of any help. The main issue, as far as I can see, is because the Service class is in essence only a conduit to cloud APIs. So there no real need to extensively mock them, by doing so, it makes it wrong and is waste of time to write tests that are 'unrealistic' tests. To make that crystal clear, I've used my own service replacing the default ones, and guess what, not matter what I do, the tests are always GREEN.

Fundamentally, once the service pattern is tested for once and for good, the API service which consist of sending a path and a hash(.to_json) and getting a response, leaves the rest to integration tests like VCR. The latter part actually tests an API works.

Also the unit test code shouldn't appear in production. Normally when packaging the gem, the test folder(s) is left aside. Well that's impossible to do because the Mock class is embedded into the Service subclasses.

So, can we, please?

[Temikus]

I think this is relevant to #12

[gildub]

@Temikus, yes removing any tests using Mock would have to be done too.

[plribeiro3000]

@gildub There is interesting points here for sure. But there is also the other side of the coin. Mock might be a pain for us to maintain in the source code but when it works (not all apis are available in mock mode) it helps who are developing using fog by a lot. I know that because i've done it before and it really helped. Another point here is that the code you were using might not be in a good shape. I've got my code right so many times because of mock, either because i was not using the right id (so the object did not exist) or just because i was using the wrong signature. There is also a lot of people out there that might be using this feature so we can't just drop it considering it is a great deal to fog and in my opinion the major advantage over other libraries where i need to craft live tests or don't have tests at all.

So i believe that if done right, the Mock feature helps a lot. What we might need is to make sure they work as they should and maybe extract Mock into fog-mock so we can load it in development and test environments only.

Also i believe that your real problem isn't the feature (Mock) but some code that is not up to date or well crafted, so removing the feature completely is not the right answer IMHO.

So i suggest we look at this from other angle. Which feature, code, service, provider did not work for you?

[gildub]

@plribeiro3000,

to clarify myself, Mock class doesn't have to disappear at all but nevertheless shouldn't be mixed with the source code and has to go where it belongs to which is into the test code along all doubles and other mocks.

Therefore :+1: to extract Mock and move it to fog-mock to load it only in dev/test. Alternatively that library could reside into test helpers.

The other part, I was complaining about is actually irrelevant here and is not an issue. I don't have any trouble with ongoing tests. Thanks for offering your help though.

[plribeiro3000]

Gotcha.

That makes sense. Would you be up to extract Mock into its own gem?

[plribeiro3000]

BTW, we can't/shouldn't move it into test helpers because that means we would have to create helpers for each one of the most used test frameworks. And i don't think we should spend time on such thing for now.

[gildub]

@plribeiro3000,

Extracting Mock is going to be an interesting task. I can start but I believe some of the core members should be involved into it too.

To start with, as a member of the fog organisation, could you please create a repository ? As for a name, a good candidate is fog-mock-core, to start the mock family group as there is going to be related projects, i.e. fog-mock-openstack and so on.

[plribeiro3000]

I believe we can start with fog-mock only since we wont need to create one for fog. The direction we are heading is to push all efforts to extracted providers, new providers need to be on its own gem from beginning and old providers are being extracted as possible. This means that at some point we will also drop fog since most use cases don't need more then 10 providers that we support so always having everything is kind of a burden, meaning we will have only extracted providers without a gem to include them all.

Regarding extraction i believe that a person that is not familiar with the codebase would be the best fit to do the job with someone from the core team helping by reviewing the changes and checking if it makes sense. This way there is less chance to be addicted to one feature or another and make a mistake.

I can help you out by reviewing your changes and trying to guide you through any difficulty you might have. What do you think?

[gildub]

@plribeiro3000,

The name and approach makes sense. You have more visibility.

For the extraction I think that's doable.

[plribeiro3000]

@gildub https://github.com/fog/fog-mock

[gildub]

@plribeiro3000,

Ok, I've pushed initial extraction onto fog-mock and on par with it I've pushed PR#200. The latter to fail until the first fog-mock gem is available.

BTW, could you please create fog/fog-mock-openstack repository and add me to it?

Thanks

[plribeiro3000]

@gildub awesome. I just reviewed it and made some comments as we need to be backwards compatible for now.

As for the new repository i'm not sure we need to go this path. This means we will double the number of repositories we already have. What i was having in mind is to change the structure from inside the providers in a way it would not load the mock class by default and that the use would need an extra require. Does that make sense to you?

[geemus]

I suspect we would probably need to do one mock-repo per provider (similar to what I think is described above). As @plribeiro3000 suggests, I think they do provide a lot of value but it certainly makes sense to me that they could be something you opt-in to instead of something you are forced to bring along. Also I would agree with @gildub that for test purposes, things like webmock may be more accurate and easier to maintain, but a lot of the value in mock is how much faster/easier end-users can get their fog integrations up and running (that is really why we built the mocks in the first place, running tests against them came later and was as much to make sure the mocks were accurate as anything else). Hope that helps, context-wise anyway. Happy to discuss and support as I'm able.

[plribeiro3000]

@geemus Yeah, with that in mind i'm not that sure with this move anymore. What do you think? Does it make sense at all?

[gildub]

@plribeiro3000,

I believe the move makes sense, if we cannot make it happen somehow then the project would risk a split (fork) because a version without Mock is going to appear.

[gildub]

Looking at moving the Mock classes for the fog-openstack project, I think there is no need for a dedicated fog-mock-openstack place-holder, at least for now. Moving the Mock classes in the test section (for instance test/lib/fog/) works quite well and keeps everything handy.

I actually wonder if the same approach shouldn't be used here too for fog-core? @plribeiro3000, I know the initial idea of having a dedicated fog-mock gem is tempting but there might not be the need for it or it might just be too early for that.

@geemus, what do you think?

[Temikus]

While I see the moving 'Mock' as a good change, I'm a bit worried that we may not have the bandwidth to support 2x amount of additional repos + their miscellaneous version issues.

I'm especially worried about requiring people submitting PR's to sync their changes between 2 repos with different release schedules.

Am I making sense? I may not be seeing something though since I haven't used Mock much.

[gildub]

@Temikus, thanks for jumping in.

Ok so another gem for Mock is overkill. Let's extract Mock classes and the related code to test/lib. That would be a good starting point and might just stay there.

Actually, there is not much code involved:

• 2 Mock classes used by ssh and scp, the 2 inbreed providers
• The Service class triaging the Mock/Real cases
• An Error class
• Some require glue
• and of course few mocking constants

[gildub]

Ok I just said test/lib meanwhile because the fog projects need that code, if using mock, it would be better in lib/fog/test_helpers/.

[gildub]

A note about SSH and SCP Mock classes which do contain Mock classes but are just as any other service provider would be. I believe they are in fog-core because there's no fog-ssh project (more likely that would have happened during the fog main project split) .

This actually raises the question: Shouldn't scp and ssh be living into a dedicated provider? This is in order to respect a plug-in approach where fog-core is only the core (as common). I guess for now they could just go in test section (but not in the test_helpers).

[gildub]

This [1] would be one way of doing this 'in-house' without using a fog-mock (or so) gem.

[1] https://github.com/fog/fog-core/pull/201

[geemus]

Could it just be in something like /lib/mock instead of test? I guess it just feels weird to put it in test as it seems to imply it's just for our tests (vs also being for end-user tests). Does that make sense? We could still skip requiring it potentially, I just don't want to put it somewhere that would be more difficult for end-users.

[plribeiro3000]

I'm rethinking this whole thing.

We are running around without going nowhere. We are changing our opinions for something that is not even the original problem. Maybe we should take one step back (or even 10) and think about it again.

With @geemus comment on end users and that Fog.mock is not for test purposes only i don't see a reason where it could not be the way it is right now. I have to also agree with @Temikus here. We don't have people neither bandwidth to double the number of repositories and the sync between apps will be a PITA.

So lets get back at the origin. Your point was to remove the Mock system. Rather then that could you please give us insight on your actual problem? So we can try to figure a better solution.

[gildub]

@plribeiro3000, Mock mixed up with the code is wrong, not matter what the angle or how many steps back you take to look at it. It's clear that's a good change but not using the gem approach.

[plribeiro3000]

I understand that but can you please tell us your issue? Mock or not to Mock is a subject we might not need to touch now if we figure out the issue that made you get into this path. Can you please share the problem?

[gildub]

@geemus, sorry for the confusion, I initially said test/lib but I realized that the core part of Mock has to reside under lib to be able to be used by other projects/providers. lib/mock or lib/fog/mock sounds good too, meanwhile I thought lib/fog/test_helpers was a good place, is it not for the end users tests you mentioned?

[gildub]

@plribeiro3000, the issue is that test code shouldn't be mixed with source code. Maybe few years back that was a good idea, and as I said, at the time I'd have thought so too, but today with more experience with TDD we know it's not a good path.

[plribeiro3000]

So is basically just a design issue, right? It sound dumb but i just want to make sure we are at the same page since initially i wrongly thought it had a bug for some reason.

[gildub]

@plribeiro3000, oh I see, yes it's a design issue. Other comments show that it's a good move. The questions that came up are about being backward compatibility and how we make it. So the change is not to remove Mock but untangle it from the source, make it on its own.

[plribeiro3000]

Nice. Thanks for the confirmation.

This is a design decision made a long time ago as you suggested and even that it might not be one of the greatest options today we need to have the end users in mind. So i wonder how they are using fog nowadays. 🤔

@geemus Can you please share some thoughts regarding this? Do you think it makes sense to move this since it might break features that are working today?

@gildub My concern now is: Does it worth to do such move? Depending on how we look at it it make sense to do the move you are suggesting but as time makes us learn the hard way we know today that one single issue on fog-core and we will make lots of people angry. 😠 ehheheh One big point to this question is that we are in the path to move to 2.0.0, deprecate tons of stuff (including old ruby versions) and maybe now is not the best time to add one more sauce to the mix. So there lies my question, should we do this and should we do it now?

@Temikus @geemus What are your thoughts on this subject?

@gildub bear with me please. This is a really important piece of the project so this might take a bit longer to move to any side it might be decided to move. Sorry about that.

[gildub]

@plribeiro3000,

First off, thank you for following-up. I perfectly understand it's important to evaluate properly the needs.

To respond to your questions/comments:

• Yes it does worth making the move (either now or through next major release if that's coming soon enough). For the same reason I've created this issue, Mock classes are in the way and not a standard practice. It's in the long term interest of fog-core to evolve along. Meanwhile, there is no need to make anyone angry, as I said, Mock is not going away it's just moved in its own library set. It's great to have them as an option.
• I'm glad to read that Version 2 is on its way. A major release is actually a very good timing, features wise. In such case I'd would completely replace the Real classes from the main code and keep Mock as optional stubs. BTW where is 2.0.0 branch, where are the discussions about the new features?

[geemus]

I can understand how this would be cleaner from a design perspective and more clearly separate concerns. So from a purity perspective, it does seem right. That said, what we have technically works, so for me personally I don't feel a strong desire to put in the work to fix it. But I could support someone else who felt more strongly. I do worry that this would establish inconsistency between the different projects though (as I suspect not all of them would be changed over, we've already seen lag between them as it is). So, in short, I'm ok with it but it doesn't seem like a high priority.

[gildub]

@plribeiro3000, what's your take on this one then? Could you also provide more details about the new major release?

[plribeiro3000]

@gildub I agree with @geemus and i don't have spare time to work on this, even to only help reviewing so i'm not sure which path to follow now.

Regarding the major release @geemus is the one to give you more info. 😉

[seanhandley]

I've actually found the mock request classes very useful when adding new OpenStack APIs lately because it gives me a place to put the example request/response code outlined in the OpenStack API documentation and see it alongside the real implementation.

I think the strength of this approach is borne out when you run your tests with and without FOG_MOCK. If it works with your mocks and with your real class then you know your mocks are a realistic representation of how the API behaves.

[geemus]

Yeah, I've certainly found it helpful in a lot of cases as well. But perhaps as @seanhandley suggests, this may be more a benefit to those of us developing the library (vs end-users). Maybe it is a question of cost/benefit at that point? ie how much value do we get and how much impact/cost does it push onto end users. I agree that it is unusual and perhaps questionable to have things structured this way, in terms of what is ideal, but given the value we see I think it might be worth just letting it be kind of weird. Thoughts?

[gildub]

Benefit Better design to allow easier/faster evolution More flexibility

Cost Move the Mock classes (I insist we do not to remove them) is not an issue (per the initial PR about it). Not doing it could cost the opposite of its advantages, less evolution, less flexibility

[geemus]

@gildub hey, thanks for continuing to discuss this with us. I'm not sure what you mean about the cost of moving the mock class not being an issue? It seems like a good deal of somewhat error-prone work to get there, to me anyway. I do agree that it may be a less ideal design though.

[gildub]

@geemus, what I meant by the cost not being an issue is, at least for the current version, Mock classed could be just moved to separate files.

[geemus]

Ah, so you mean everything would still work as it does today. I guess my thought was just that the cost of doing all the moving is not nothing (though the risk should be pretty minimal, as you suggest).

[gildub]

I understand the requirement for current version to have impact-less changes or no change at all. And on the other hand, a new major version could start with the mock classes being separate and even optional. Would that fit everyone?

[geemus]

Yeah, a new new version could certainly be an inflection point for this.

[gildub]

Happy new year! Fantastic. When do you think that could happen?

[geemus]

@gildub great question. I don't have an explicit plan at this point (and unfortunately have been rather swamped with lots of other things).

[plribeiro3000]

Ok. Lets get back at this issue so we can move forward after we make a decision and draw a plan of work if needed.

Since the current implementation works, i believe the best way to decide it is to leave as is if we don't find good reasons to change it, of if we don't get to an agreement.

My points:

• Mocking is a great way to start developing a new app without worrying too much about doing unwanted requests at the provider (e.g. creating a paid vm)
• I understand some time is a PITA to maintain both the real and mock classes and to make sure both behave the same way
• If we decide to change it, we need to change in all providers otherwise it will be unexpected by the end user that one provider does one work in a different way of the other ones and this will be a great effort

cc/ @geemus @gildub

[geemus]

Yeah, it is a pain. The original idea had been that, though it is a pain, we as the maintainers are more expert and can create more reasonable mocks than a new user would be able to. So I had thought it was worth the pain, but it may be worth revisiting that to see if things have changed enough to alter our approach.

[gildub]

Yeah @geemus, well said, and alike other people in my team using fog-openstack. To such a point that I've been thinking about monkey patching fog-core to be able to remove Real class: https://github.com/fog/fog-core/blob/master/lib/fog/core/service.rb#L115.

In short, l understand the feature itself to help with simulation but it should move out of the way (in a lib or gem). The high risk here is a fork for fog-openstack to break free.

[geemus]

@gildub thanks for taking time to talk through this. Could you elaborate on your concern around a fork breaking free? Do you mean that it would be able to develop/move faster and we would fall behind (because working on mocks would slow us down too much)?

[gildub]

@geemus, yes there is the Mock/Real pain structure which is cluttering the code and slows us down to re-factor when needed but not so much when adding new requests although that keeps making things worse.

Beyond that, as you know, there is the service issue https://github.com/fog/fog-core/blob/master/lib/fog/core/service.rb#L52. I was hoping v2.0 would have started addressing it including the Mock/Real and not have to pull it from the provider end.

Effectively, those re-factor requirements are driven by fog-openstack enhancements.

I understand it's really hard for fog-core because its the common denominator between different providers which makes changes tricky, which ironically brings us back to square one.

Trying to progress constructively on this issue, it would be great if we could agree to the following:

• Move Mock classes to lib/mock in the short term (no impact to fog-core, that's on provider ends) until a better name space and plug-in solution is found. That seems to be a win-win to satisfy both parties to keep the feature and at same time have it on the side.
• Start a re-factor work to offer the Mock feature as a "plug-in" and allow to use the services classes without relying on Real class name to un-clutter the name-space.

[plribeiro3000]

Maintainers of other providers should also chime in. This change will affect everyone so we need everyone aboard.

cc/ @fog/fog-google @fog/fog-aws @fog/fog-ovirt @fog/fog-vsphere