Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
FEATURE: Allow asserting on requests in tests #1069
Changes from 11 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
There are no files selected for viewing
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.
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 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 comment
The 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
store_to_array
method.I think it might be helpful to add a comment explaining the intention behind using
object_id
and_id2ref
. This would really help to understand the reasoning behind this approach.That said, I believe we might be able to simplify this code. I'm not sure whether we really need
object_id
and_id2ref
at all. From my understanding,array
is just storing references to the original objects, not clones.I think the following achieves the same result:
This version uses a hash (
@request_objects
) to store the original request signature objects with same#hash
result. It maintains the original object without the need forobject_id
.It should be more efficient as it avoids the overhead of
ObjectSpace._id2ref
and potentialRangeError
rescues,though I don't know how much faster that is.
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 comment
The 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...