FEATURE: Allow asserting on requests in tests

[johngallagher]

Context

@jamesshore has created the concept of "Testing With Nullables".

I’ve figured out another way. A way that doesn’t use end-to-end tests, doesn’t use mocks, doesn’t ignore infrastructure, doesn’t require a rewrite. It’s something you can start doing today, and it gives you the speed, reliability, and maintainability of unit tests with the power of end-to-end tests.

A small snippet:

it("reads command-line argument, transform it with ROT-13, and writes result", () => {
  const { output } = run({ args: [ "my input" ] });
  assert.deepEqual(output.data, [ "zl vachg\n" ];
});

function run({ args = [] } = {}) {
 const commandLine = CommandLine.createNull({ args });
 const output = commandLine.trackOutput();

 const app = new App(commandLine);
 app.run();

 return { output };
}

I've been experimenting with these techniques in various codebases and love the code that results.

Why

This style of assertions, whilst not adhering to James' full pattern, is a thin layer on top of what Webmock is already doing - @jamesshore calls it "Output Tracking"

Example

Let's say we're doing an API request to Cloudflare to get IP blocking rules.

Before

  it "makes a get request to Cloudflare to get the rules" do
    stub_request(:get, "www.cloudflare.com/api/v2/rules").to_return_json(body: [{...}])
    subject.call
    expect(WebMock).to have_requested(:get, "www.cloudflare.com/api/v2/rules").
      with(query: {"ip" => "2.5.4.3"})
  end

Strengths

• Leans into the Ruby and RSpec metaprogramming conventions
• Reads like English

Weaknesses

• Maintaining a #have_requested RSpec matcher means more code
• Custom matchers means writing readable failure messages
• RSpec magic means that if the custom matcher fails it can be difficult to debug

After

  it "makes a get request to Cloudflare to get the rules" do
    stub_request(:get, "www.cloudflare.com/api/v2/rules").to_return_json(body: [{...}])
    subject.call
    expect(requests_made.count).to eq(1)
    expect(requests_made.first.method).to eq(:get)
    expect(requests_made.first.uri.host).to eq("www.cloudflare.com")
    expect(requests_made.first.query).to eq(ip: "2.5.4.3")
  end

Strengths

• Plain old Ruby
• Works with Minitest with no extra code or magic needed
• Allows asserting on order of requests
• Opens up options for extra helper methods
• Reduces coupling to Webmock - we could use another mocking library, implement #requests_made and we'd be good

Weaknesses

• #requests_made needs a mixin to work
• Can be more verbose (this can be mitigated by writing helper methods)

After - Cleaner

  it "makes a get request to Cloudflare to get the rules" do
    stub_request(:get, "www.cloudflare.com/api/v2/rules").to_return_json(body: [{...}])
    subject.call
    expect(cloudflare_requests_made.count).to eq(1)
    expect(cloudflare_requests_made.first.method).to eq(:get)
    expect(cloudflare_requests_made.first.query).to eq(ip: "2.5.4.3")
  end

  # private method for more readability and resilience
  def cloudflare_requests_made
    requests_made.select { |r| r.uri.host == "www.cloudflare.com" }
  end

How

• Adjust the HashCounter class to store requests in an array (see comment for details)
• Add an extra convenience parsed_json_body method onto Webmock::RequestSignature
• Add #requests_made onto the registry

[johngallagher]

@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.

[bblimke]

@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:

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 (@request_objects) to store the original request signature objects with same #hash result. It maintains the original object without the need for object_id.

It should be more efficient as it avoids the overhead of ObjectSpace._id2ref and potential RangeError 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.

[johngallagher]

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...

[johngallagher]

Gah @bblimke all the tests are failing and I realised I need to do some documentation (I'll do that in a separate PR if that's OK?)

I'll come back to this and debug.

[johngallagher]

I'm getting a ton of unrelated CI failures locally - no idea what's going on here.

These tests are all failing on master branch:

Happy to pair with you some time @bblimke to make this all green again.

I'm going to leave this for now as I'm not best placed to do a deep dive into the codebase...

[bblimke]

@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.

[johngallagher]

@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 a RequestStore or something similar... although that's literally what RequestRegistry does I guess... so I'll look into that. Thanks! Great idea.

[johngallagher]

@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:

- WebMock::RequestRegistry.instance.requested_signatures.put(request_signature)
+ WebMock::RequestRegistry.instance.put(request_signature)

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):

class WebMock::RequestRegistry
  def initialize
    @hash_counter_store = HashCounterStore.new,
    @array_store = ArrayStore.new
  end

  def put(request_signature)
    @hash_counter_store.put(request_signature)
    @array_store.put(request_signature)
  end

  def get(request_signature)
    @array_store.get(request_signature)
  end

  def to_a
    @array_store.to_a
  end

  def to_h
    @hash_counter_store.to_h
  end
  # ...
end

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:

1. Continue as I am munging it all into HashCounter
2. Refactor before adding this functionality, then revisit this PR (defo a breaking change here!)

WDYT?

[johngallagher]

@bblimke nudge :)

[bblimke]

@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.

[bblimke]

@johngallagher all tests in master branch are passing now, therefore feel free to marge master branch to this one.

[bblimke]

I'm getting a ton of unrelated CI failures locally - no idea what's going on here.

These tests are all failing on master branch:

Tests on master branch is passing now, if you would like to rebase your branch.