search

carta / styleguide

Carta's style guides
26 stars 5 forks source link

Be consistent when mocking functions & properties #11

Closed tizz98 closed 6 years ago

tizz98 commented 6 years ago

Proposed: https://github.com/carta/styleguide/issues/6#issuecomment-373207095

I think we should be consistent with how functions are mocked, whether at the class level or instance level.

bhardin commented 6 years ago

I don't know about this. My question revolves around why you need to mock a function at the class level. In OOP, everything should be an instance. Class level functions should be rare.

jourdanrodrigues commented 6 years ago

@tizz98 I think this is adding complexity to something that shouldn't be complex.

The only reason we use patch.object is to encapsulate the mock within the test scope. When we consider local instances, doesn't make sense to do it on them because they'll exist only until the scope ends, as well as its mocks.

As Brett said, the cases where you need to mock classes' attributes should be (and are) rare. Today it's restricted to static/class methods (which, as far as I know, we're trying to remove), third party classes/objects and QuerySets, as well as custom QuerySet methods.

mannysz commented 6 years ago

If we should only mock instance attributes, and there is no reason to mock instance attributes using patch.object, should we stop using it (or just use in cases the mocked object is not accessible from the outer scope)?

mannysz commented 6 years ago

I also do @emyller, and that's because we'll have two different ways to mock properties (class and instance attributes) inside our code. Since we should never assign a class attribute because it will change the same class for all the following tests, we need to ensure all changes are being scoped.

jourdanrodrigues commented 6 years ago

Looking at the patch method's source code, I'd argue that there is too much happening here to use it on something that doesn't need the control provided by patch.

I agree that people may do it wrong, though. That's why the reviews led by Brett are so strict. Maybe making it more explicit in the doc should fix this possibility?

emyller commented 6 years ago

@jourdanrodrigues My say is: the mock lib provides a way to patch things; given that, we shouldn't need to patch things manually. Using mock.patch (or mocker.patch) is simpler, safer, recommended and we all know what to expect. 😉

jourdanrodrigues commented 6 years ago

@emyller I'm not talking about what it is for, it patch things very well, which is why we use it. What I'm talking about is its use-cases. I don't know what you meant with "patch things manually", because we still have to type the mock with the patch.object (even more), so we're still the ones who mock, not the package.

Again, we're using mocker.patch.object today only to keep classes/packages mocks in the test scope. Local instances don't need that because they already exist only inside the test scope.

But since I'm the only one here that don't agree with this, I'll be less passionate about it and just quote Brett: if it is fast (less than 0.1s as off now), go for it.

mannysz commented 6 years ago

Why didn't we merge this?