-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEATURE: Allow asserting on requests in tests #1069
base: master
Are you sure you want to change the base?
Changes from all commits
27e3095
3245079
7f93ba1
a5ab52a
88e9664
b366874
fbf292f
c8815ee
fd04cbc
6890a01
2b8a895
3eaead4
cb5c40c
577f64e
6705f53
088aa26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,22 +5,32 @@ | |
module WebMock | ||
module Util | ||
class HashCounter | ||
attr_accessor :hash | ||
attr_accessor :hash, :array | ||
|
||
def initialize | ||
self.hash = Hash.new(0) | ||
@order = {} | ||
@max = 0 | ||
@lock = ::Mutex.new | ||
self.array = [] | ||
@request_objects = {} | ||
end | ||
|
||
def put(key, num=1) | ||
@lock.synchronize do | ||
store_to_array(key, num) | ||
hash[key] += num | ||
@order[key] = @max += 1 | ||
end | ||
end | ||
|
||
def store_to_array(key, num) | ||
stored_object = @request_objects[key] ||= key | ||
num.times do | ||
array << stored_object | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bblimke this is missing any comments to explain what's going on - maybe worth me adding some? For others - doing this so that we are just referencing existing requests in the array, rather than creating new objects. This is so that if we have massive numbers of requests that have the same signature, we're not bloating memory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johngallagher Thanks for your work on this. I've been looking at the I think it might be helpful to add a comment explaining the intention behind using That said, I believe we might be able to simplify this code. I'm not sure whether we really need I think the following achieves the same result: def store_to_array(key, num)
@request_objects ||= {}
stored_object = @request_objects[key] ||= key
num.times do
array << stored_object
end
end This version uses a hash ( It should be more efficient as it avoids the overhead of Can you think of any scenarios where the original implementation behaves differently from this proposed one? What are your thoughts on this? I wonder if this can me optimised even further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a much nicer solution than mine! I can't think of any scenarios just now but I'll think on it over the next few days... |
||
|
||
def get(key) | ||
@lock.synchronize do | ||
hash[key] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johngallagher I believe we should separate the array of requests from the HashCounter. The HashCounter has a specific, single responsibility, and adding an array of requests expands its scope beyond its intended purpose.
Instead, I suggest moving the ordered list of requests up to the RequestRegistry object. This way, the RequestRegistry would manage both the HashCounter and the array of requests, maintaining a clearer separation of concerns.
What are your thoughts on this restructuring? It would allow the HashCounter to remain focused on its core functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bblimke yup, that would work better. I do think that having all this cruft in the
RequestRegistry
might clutter it up, so it might be aRequestStore
or something similar... although that's literally whatRequestRegistry
does I guess... so I'll look into that. Thanks! Great idea.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bblimke aha - I remember why I didn't put it into a separate object now. 💡
It's because of the design -
HashCounter
is exposed directly in lots of places like so:https://github.com/johngallagher/webmock/blob/088aa2678f7efb34e910e8b430b54980283b9485/lib/webmock/http_lib_adapters/curb_adapter.rb/#L61
Personally, I'd rather encapsulate how the registry stores requests and hide that from clients.
That would mean we could have one store, two or more. But given how it's done, without a huge refactoring, I don't see how we'd get this in.
I'd be happy to take on that refactoring, to be clear - I love me some refactoring!
It'd be:
And the
requested_signatures
would be entirely private. We could have two different store classes that way and do something like (sketching it out roughly):This would give us duck typing for stores, make their internals private and allow us to reach into the relevant store when we wanted to pluck out requests that were stored in a specific format.
To recap:
HashCounter
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bblimke nudge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johngallagher Thanks for the reminder. I agree that RequestRegistry internals should be private, with no other object having direct access to the hash counter or the array. I like your solution with two separate stores: hash_counter_store and array_store.
I'm not sure if this is a breaking change, since RequestRegistry isn't supposed to be used directly. It's not part of WebMock::API, which is the only versioned interface. However, there's a likelihood that some people have accessed RequestRegistry directly.
I don't mind releasing a new major version though, if we decide it's necessary.