Fixes and refactor

[maraujop]

Hello,

We've needed to do several changes to make mockfacebook work with our testing setup and current Facebook's API. Here are the commits introduced in the project.

There are some new features, some fixes and some refactoring going on. In the end we've found mockfacebook's architecture hard to extend and very complicated.

Just in case you want to merge some of this stuff in.

Cheers,
Miguel

[snarfed]

hi miguel, thanks for the contributions! and apologies for the difficulty. mockfacebook grew organically out of a gross hack, so you're right, it's definitely not designed as cleanly as it could be. i also only needed it for one project, and haven't used it much afterward since i don't work on facebook apps much, so it hasn't had the love it needs.

i'll take a look at these within a day or too, but i'm sure i'll happily merge them with only minor changes, if any. the one thing i would ask is that you add unit tests for the new features and bug fixes. mind doing that?

[maraujop]

Hi Ryan,

Thanks, I understand, no problem.

Currently we are developing at the highest speed possible, so due to time constraints I'm sorry but I don't have time to write those tests. Actually, some of the fixes, are not real fixes, but workarounds to get it working.

Thanks for releasing your project, it's helping us speeding up testing.

Cheers,
Miguel

[snarfed]

re time for unit tests, ok. personally, i've found that skipping tests actually costs me more time in the long run, due to more bugs and time consuming debugging, decreased confidence and tendency to avoid refactoring, etc.
regardless, it's your call for your own work, of course.

for mockfacebook, though, i'd like unit tests for all functionality, so we may need to wait on some of these commits until you (or i) write tests for them.

[maraujop]

Let me explain myself a little bit.

I'm totally in favor of testing when developing a project. I usually put a lot of time and effort in keeping up with testing, as you say it pays off.

The problem now, is that we are overwhelmed with work, and we are using our fork of mockfacebook for speeding up tests. Our whole battery of tests was working against facebook API through HTTP. It's now working against mockfacebook, so I'm quite confident it's good enough.

If mockfacebook fails in the future, we will have to eat our own commitment of not working on tests and suffer it.

• As you've seen, we've added support to mimic facebook test users, this helped us to not change a single line of our testing suite and attack mockfacebook directly.
• me is now dynamic, the same way facebook behaves. It finds out current user, from a new mapping between user_id and token.
• We've also fixed some of the responses, which had duplicates that were wrong or were not wrapped in a data keyword.
• We've also added support for fields parameter, otherwise our tests would break, as our core expects response to be fields filtered.
• Adding requests is not necessary, true. All I can say, is that I hate httplib2 and urllib way of doing things. Python requests has neat syntax and is a great human HTTP library. It also helped us debug error responses from facebook we were getting and clean code.
• And yes, catch-all except clauses are wrong, they are evil. When I added that, I was just trying to figure out if I would be able to get mockfacebook working again.

If we have time in the future, I will come back and write those tests. I understand you don't want to merge this in as it is. I would do the same. But I thought maybe others find this useful meanwhile.

Cheers,
Miguel

[snarfed]

thanks for the explanation! that all makes sense. i definitely don't think you should have to test your tests, even though some people do, so from your point of view, where mockfacebook is just part of your own test infrastructure, i understand why you haven't prioritized unit tests for these fixes and new features.

i'd actually be ok with merging most of these in as is if they have TODO comments to add tests later. if you add those TODO comments, and add exception type(s) to the except clause and remove the requests lib commit (i'd like to think about that more), i'll happily merge this pull request.