Eliminate #any_instance_of in logging specs #15058
Conversation
|
Also customary ✂️ ✂️ ✂️ |
|
@abellotti bump |
|
wasn't too crazy with the string matching in the specs, vs. the previous hash ways. |
imtayadeway
force-pushed
the
api/refactor/logging-specs
branch
from
eb0e7e5
to
f6bc34d
Compare
|
@abellotti would you care to say why? And could you weigh any objections against the benefits I've outlined in #15058 (comment) ? I've rebased and updated a little bit since you last looked at it (principally to switch from mocking to verifying against an injected log device), although the expectations remain basically the same. Might allay some of your concerns though as I've tried to make the expectations as order-independent as possible. I think generally the result is a lot better though, because now these tests are testing external behavior, not private implementation details. |
|
I like testing at the boundary. And testing in a way that is order independent. |
The performance characteristics for chaining matchers here appears to be exponential (see rspec/rspec-mocks#1158), so using a single regex here instead.
imtayadeway
force-pushed
the
api/refactor/logging-specs
branch
from
f6bc34d
to
aa602bc
Compare
|
Checked commits imtayadeway/manageiq@fa02873~...aa602bc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@abellotti bump |
|
The |
With the collision of ManageIQ#15058 and ManageIQ#15392 these specs are now failing. Since there's no way to sensibly implement `#reopen` on a multicast logger opaquely, instead we'll add a logger to the multicast loggers and verify against that.
By mocking closer to the boundary (i.e. theBy injecting a$api_logitself)StringIOinto the $api_log and making expectations on it, we can decouple these tests from some implementation details that required us to use#any_instance_of. The result I think is greater confidence in these tests, greater visibility in what's being logged from the test body, and looser coupling between the test and its implementation, freeing us to refactor some of the logging later if needed.Also, by first stubbing the logger withWe also no longer required to specify every message that the logger receives (in the right order) - we can instead search through the log device to find only what is relevant to the example at hand. This also removes some constants from the example group which I know will make @kbrock happy 😁allow .... to receive(:info), we no longer have to specify every message it receives, giving the tests greater readability.@miq-bot add-label api, test, refactoring
@miq-bot assign @abellotti