Return 404 from GET Bindings for Expired Bindings #1355
Conversation
kyma-bot
added
the
size/L
internal/broker/bind_get_test.go
Outdated
|
|
||
| t.Run("should return 404 code for the expired binding", func(t *testing.T) { | ||
| // given | ||
| mockBindings := new(mocks.Bindings) |
There was a problem hiding this comment.
just a question, why just not use in-memory implementation?
There was a problem hiding this comment.
- With mocks I am guaranteed the bindings' "Get" method is called.
- I do not need to check if anything has been created in the test, it just for quick return and assertion that 404 is returned when for prefabricated binding.
There was a problem hiding this comment.
We are using in memory implementation in other unit test which works fine, I don't like to introduce a different solution which does not give us any special value. Such pattern is also used in fake kubernetes client. Mocking database can be risky.
The title of the test is "should return 404 code for the expired binding", not "should call 'get' method", it is not important in my opinion.
There was a problem hiding this comment.
I still find value in asserting execution. In order to meet the argument of unnecessary test modifications when interface changes and mock regeneration is required I would suggest to treat interface as something that rarely should change or rely on new interfaces in new logic.
Done as requested though. Let's discuss offline what are the possibilities to use spies or sth else to assert method executions in our project.
There was a problem hiding this comment.
It's not about interface changing but implementation changing. We relied on implementation (test relied on the knowledge how logic is implemented) which in this case I found excessive, or even inappropriate.
There was a problem hiding this comment.
No, that is not true. The test contained assertion on logic so that in the future if our "black box" called bindings multiple times we would know that it is not aligned with initial assumptions. Think about it not as an assertion if specific instruction that the unit uses but as requirement introduced because unit relies on external dependency in the form of "Bindings" interface. In the current form, we could remove Bindings dependency, return 404 with correct message from the unit and the test would be green. The Bindings interface is external, not internal to the tested unit.
There was a problem hiding this comment.
I beg to disagree. Test assumed that implementation calls Get method once. This is not part of contract.
There was a problem hiding this comment.
Assuming that we test contract and use black-box testing, we do not need to know that there is any external dependency, we do not need to know how it is implemented, how many invocations of any kind is made.
We know only, that if the binding in question is expired, we get 404.
There was a problem hiding this comment.
We can testing if there is only one Get call on repository, but... then we should test if the binding manager deletes... deployments or something else by accident. We cannot test everything! such tests would be a problem, not a help
kyma-bot
added
size/M
Description
Changes proposed in this pull request:
Related issue(s)
#284