Be consistent when mocking functions & properties

[tizz98]

Proposed: #6 (comment)

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

[bhardin]

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]

@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]

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)?

[emyller]

I agree with @tizz98's proposal, for the sake of consistency. Mocking instance members isn't wrong, but it looks similar to mocking class members and that alone can help developers make copy-paste mistakes.

• patch and patch.object are always safe and easy to follow and control.
• Explicit using Mock (and its children) looks more consistent for creating things instead of overriding them.

[mannysz]

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]

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]

@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]

@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]

Why didn't we merge this?