refactor/provider_key_change_use_case#16
Conversation
The ProviderKeyChangeUseCase has been refactored to use the Service class methods to clear the cache, removing duplication
miguelsorianod
changed the title
| def exercise_provider_service_memoizer(provider_key, service_id) | ||
| # Call the Service class methods just | ||
| # to force the memoization of them with | ||
| # specific values |
There was a problem hiding this comment.
comment lines can be longer here
| end | ||
|
|
||
| it 'clears the Service cache keys of the old provider key' do | ||
| service_ids_with_old_key.each do |service_id| |
There was a problem hiding this comment.
this can be described in a new context or describe block if you split these examples in one part for the "general" specs and another one for the memoization stuff, and this way you would not need to insert use_case.process in all the other examples as a consequence of removing it from the before block (you would have two describe/context blocks, and each with independent before blocks if you do not nest them).
unleashed
left a comment
unleashed
left a comment
There was a problem hiding this comment.
LGTM. I am not a fan of having to spec memoization effects, but it is true that we are already caring about clearing some entries and some other places also test this. Not made my mind yet on whether we should be generally doing that and if so whether it should be here, but it's ok atm.
| end | ||
|
|
||
| def build_provider_service_memoizer_keys(provider_key, service_id) | ||
| memoizer_keys = Memoizer.build_keys_for_class(Service, |
There was a problem hiding this comment.
no need to define memoizer_keys. It's not used anywhere.
There was a problem hiding this comment.
That's true. I'll remove it
| exercise_provider_service_memoizer(new_key, service_id) | ||
| memoizer_keys = build_provider_service_memoizer_keys(new_key, service_id) | ||
|
|
||
| memoizer_keys.each do |memoized_key| |
There was a problem hiding this comment.
To test a change in the state of an object you can use the Rspec change matcher: https://relishapp.com/rspec/rspec-expectations/v/3-7/docs/built-in-matchers/change-matcher
In this case, we want to check how all the elements of an array changed, so using the change matcher becomes a bit more complex:
expect { use_case.process }
.to change { memoizer_keys.count { |k| Memoizer.memoized?(k) } }
.from(memoizer_keys.size)
.to(0)
I'm not proposing to make the change, just wanted to let you know that the matcher exists.
There was a problem hiding this comment.
Why not proposing it? Seems sensible.
There was a problem hiding this comment.
Good to know. Thank you for the information 👍
There was a problem hiding this comment.
@unleashed I like it and it's what I would use, but it's just a matter of preference. I'm OK with both solutions.
There was a problem hiding this comment.
It's not only a matter of preference: it vastly simplifies the code, leaving less room for error, and it reads way better.
There was a problem hiding this comment.
I have changed it and now uses the change matcher
miguelsorianod
force-pushed
the
refactor/ProviderKeyChangeUseCase
branch
from
d237d6a to
a14d5b3
Compare
Add tests to check if the memoizer cache entries are cleared when performing a provider key change
miguelsorianod
force-pushed
the
refactor/ProviderKeyChangeUseCase
branch
from
a14d5b3 to
a380570
Compare
|
bors r=@davidor,@unleashed |
Build succeeded |
3 participants
The ProviderKeyChangeUseCase has been refactored to use the
Service class methods to clear the cache, removing duplication