Conversation
test/http_test_client.rb
Outdated
|
|
||
| def get(_url) | ||
| def get(url) | ||
| self.class.last_url = url |
There was a problem hiding this comment.
I don't think this is a thread-safe implementation. If you want to get rid of the send methods in the tests I might recommend a spy that you can later assert on, some kind of UrlAssertHTTPTestClient that would expose the information you want, or possibly extracting the URL construction into a separate object that can be tested in isolation.
Overall, I don't know that it really matters though. There is no behavioral change in this PR. The send method is certainly not a good practice, but it's contained within the test for its class only - so I don't particularly see it as a big problem. Under normal circumstances we might expose this method publicly, but I don't want to make this part of the public API.
There was a problem hiding this comment.
It's rewrited with mocks! What do you think now?
Please, don't test private methods via `send`. Use `Minitest::Mock`.
AlexWayfer
force-pushed
the
improve-url-testing
branch
from
760f4fd to
1c9bfca
Compare
panthomakos
left a comment
panthomakos
left a comment
There was a problem hiding this comment.
I don't think the elimination of send in these specs is worth the extra work in maintaining and understanding the mocks.
| :call, "/maps/api/timezone/json?#{params.join('&')}", %w[123 123] | ||
| ) | ||
|
|
||
| mine.stub :url, url_method do |
There was a problem hiding this comment.
You still end up asserting on the private method call here so I don't see the benefit of this approach. The original spec (w/o the mocks) is easier to read.
There was a problem hiding this comment.
As you wish. Mocks is common practice. They're more complex in Minitest than in RSpec, yes.
2 participants
Please, don't test private methods via
send.