Define Interactor keyword arguments explicitly to improve testability #21
Conversation
dostu
force-pushed
the
feature/keyword_params_strict_args
branch
2 times, most recently
from
49d9270
to
1846ba1
Compare
dostu
changed the title
dostu
force-pushed
the
feature/keyword_params_strict_args
branch
from
1846ba1
to
ee98de2
Compare
| @@ -26,34 +26,38 @@ def self.attr_readers(target_class, attributes) | |||
|
|
|||
| def self.methods_with_params(attributes = []) | |||
| signature = attributes.join(', ') | |||
| initializer_params = signature | |||
There was a problem hiding this comment.
Not strictly necessary, I would assume this is to keep symmetry with .methods_with_keywords which also has both of these variables.
|
I'd like to revive this initiative as we've yet again had a problem with stubbing interactors incorrectly, and this change would have prevented the issue in the first place. If there's enough interest, I volunteer to drive this to completion. |
@andriusm If we define the parameters of |
|
@oleg-vinted I mean, you can do that if you really want, but I wonder whether this is the correct direction in the first place. Mocking interactor classes always seemed shady to me. It's the core of business logic, why would this be mocked. If one interactor class is calling other interactors and some of them are mocked in tests then I don't think the test is serving its purpose of protecting from changes to interactors that are a level below. Our interactors don't usually return a value or throw errors (except for extreme cases), they usually fail silently, so I think mocking them provides a false sense of security because all we assert is that something is invoked and we don't alter the execution path. My perspective is that an interactor should not invoke other interactors and if there's generic common code that needs to be executed for multiple interactors, it should be a helper function. I know it's not the case right now, but I'm not in favor of lessening the pain and making it easier to make poor choices. |
|
Strictly speaking, interactors calling each other transitively wasn't the cause of the problem I mentioned in the comment above, it was a job calling an interactor that performs the necessary operation. Interactor's parameters were changed, and job tests didn't fail because the invocation was stubbed, and the mocking library could not validate if the parameters were correct. Even then, I'm not sure what alternative you're proposing here other than keeping things as is. |
|
The alternative I would prefer is to enforce that interactors are not mocked in tests. But I see you're determined to push this one through. |
Similar to #20
We could define keyword params methods similarly to plain params. This would improve the testability of those methods too.
Currently interactor methods accept a hash so this would be a breaking change. But there are not that many places where we explicitly pass in a hash in core. Here are the required changes: https://github.com/vinted/core/pull/75828
@vinted/platform-backend @oleg-vinted