Verify calls to the registered end points have been called [discussion]

[WestDiscGolf]

There are times where we are setting up a number of requests to match in a test and as part of using the tooling to write the functionality we want to know when the setup calls have not been called when they should have been. So it can be used in a "test first" type of a way to make sure the setup urls are called when the fix has been developed. The ability to throw exceptions when calls are made which aren't configured is great, it's the opposite side of that functionality as I want it to fail or at least check when they're configured and not called.

Describe the solution you'd like
Some sort of public API which allows for verifying the registered calls have been called as part of the request and/or a way to verify all the calls which have been registered have been called.

Initial thoughts would be similar to the Verify behaviour functionality on Moq - https://github.com/moq/moq4/blob/7848788b5968eb29056599db91f9e9b9d16f9ba9/src/Moq/Mock.cs#L255

Describe alternatives you've considered
I was thinking you could create custom content predicates which when matched could keep track of the url and request. Then using the lookup functionality at the end of the processing you could manually check that all the entries had been called as expected. The downside of this it would be custom for each test and would have to be manually setup each time.

I was thinking maybe it could be based off the key creation routine and how the registrations are added to the mapping dictionary and then when they are called through the GetResponseAsync method it could be set as matched. Then could either pass in the builder instance for the request (so it could generate the key again) to check, or some other mechanism to verify that the call had been requested.

The other check that it would need to do would be the ability to use some sort of predicate to make sure the correct call had been requested. For example when the same url needs to be called multiple times with different payloads. So I was thinking it could interrogate the payload as well so you could match it in the same way that you can register predicates in ForContent to match on the content as well as the url etc.

Maybe add an override to the Register method to something like ...

public HttpClientInterceptorOptions Register(HttpRequestInterceptionBuilder builder, out string registrationKey)

So on each registration you can get access to the key value which has been used so you can then request back in later but not overly a fan of this API change.

Thoughts

Just wanted to add this here in case there was an appetite to do something like this. I would be happy to contribute a PR if I could get some guidance on naming and approach to make sure it fitted with the plan for the library.

[martincostello]

Hey Adam - I'll have more of a think about this tomorrow over the next week or so, but initial thoughts are:

1. I have nothing against the idea of a sort of Moq-esque Verify()/Verifiable() functionality being made available.
2. I don't think we'd want to expose the internal matching keys implementation out to the public API surface as it would make it harder to change the internals in the future to support emerging future use cases we don't know about yet.

Maybe it could be done in a way where the InterceptingHttpMessageHandler "told" the HttpClientInterceptorOptions that a particular request had been matched, and then it just did whatever internally to mark the fact it had been matched.

https://github.com/justeat/httpclient-interception/blob/f7b1a10a99e37caf12385e35f178681d526dfe60/src/HttpClientInterception/InterceptingHttpMessageHandler.cs#L57-L67

The downside to that, like you allude to, might be that this would run matching delegates a second time and break tests that are relying on the number of times they're invoked (like an HTTP 429 test) or something like that.

In terms of current plans, there's just a bug fix I need to come back to (#360) to investigate more, plus updating to the .NET 6 SDK next month, so I don't think you'd be tripping over anything.

[WestDiscGolf]

So I've been having a think about this and I thought "what is there already in place that we could key off to check a specific request was called?" and I remembered that the BundleItem has an Id property. So why can't we use that? It's already optional on a bundle item and then we could extend the concept.

Initial thoughts.

Add something like the following to the HttpRequestInterceptionBuilder so the id can be set through the builder pattern in code.

        /// <summary>
        /// Add a reference id to the setup.
        /// </summary>
        /// <param name="id">The id of the request to match.</param>
        /// <returns>
        /// The current <see cref="HttpRequestInterceptionBuilder"/>.
        /// </returns>
        public HttpRequestInterceptionBuilder ForId(string id)
        {
            _id = id;
            return this;
        }

The id is passed into the HttpInterceptionResponse when the Build method is called.

The HttpClientInterceptorOptions would then keep track of the internals some how. Currently thinking some sort of id mapping lookup between "bundle id" and "internal generated key" so then the internal key stays internal and can be used with the _mappings dictionary.

And then somewhere around here ...

https://github.com/justeat/httpclient-interception/blob/main/src/HttpClientInterception/HttpClientInterceptorOptions.cs#L502

... we can then mark the found response that it has been called.

Then using the mappings and keys we can find the related response and check the value and then throw an exception and/or return false depending on which way we want to run Verify.

Verify methods would be on the HttpClientInterceptorOptions and do something like this ...

        /// <summary>
        /// Verify that the specified request id has been called.
        /// </summary>
        /// <param name="id">request id to check.</param>
        public void Verify(string id)
        {
            // todo: think about using the case insensitive flag to drive the comparison
            var key = _idMappings.SingleOrDefault(x => x.Value.Equals(id, StringComparison.OrdinalIgnoreCase)).Key;

            // todo: error about trying to verify on a key which hasn't been set to verify on?
            if (_mappings.TryGetValue(key, out var response))
            {
                if (!response.Called)
                {
                    throw new Exception("not called?!");
                }
            }
        }

Yes its very rough, just trying get the API right and then will sort out the inners 😄

Thoughts?

[martincostello]

Sorry for taking a while to look at this properly.

At a high-level it sounds fine to use the ID to verify the calls made. I wonder if there might be some rough edges for multiple calls of a match though, so maybe there's some scope for tracking the counts internally (even if not exposed), but then if wasn't too difficult to add in you could also do something like Verify(string id, int times), similar to Moq.

Without trying to implement it the other difficulty there might be would be getting the message handler to definitely get the fact it was used recorded because of the support for conditionally inspecting the matches with code via callbacks and rejecting them.

The only other foot-gun I can think of is re-using a builder to mutate similar calls as you register them, meaning you might end up with an ID being reused (which you can do yourself today by accident already) meaning some potential UX-y confusion with verification counts not matching what the developer expected.

I guess those potential difficulties would be born out or disproved in trying to code up a proof-of-concept!

In either case, I think whatever the internals are, the public API surface area additions should be minimal. I wouldn't want to go down the route of ending up replicating lots of assertion-library-esque features in the library.

[WestDiscGolf]

No need to apologise 😄 Agreed on the not replicating the mocking/assertion library-esque features. A lot of effort for little gain and it's not what the library is for.

How I'm doing it at the moment within the current constraints of the public API is by adding in an additional Func<bool> which is used inside of a custom ContentPredicate. Once the predicate is successfully matched, and before it returns true for the continuation of the processing, this additional func lambda is called. This is specified where the builder options are constructed.

A different option could be making this type of structure a first class citizen of the API instead. So as part of the registration of the options you could register an optional Action which is executed when the match has been called.

        public HttpRequestInterceptionBuilder WithVerify(Action onCompletionVerify)
        {
            _verify = onCompletionVerify;
            return this;
        }

or

        public HttpRequestInterceptionBuilder WithVerify(Action<Task> onCompletionVerify)
        {
            _verify = onCompletionVerify;
            return this;
        }

And this then gets passed through to the constructed HttpInterceptionResponse instance.

Once the match is found here - https://github.com/justeat/httpclient-interception/blob/main/src/HttpClientInterception/HttpClientInterceptorOptions.cs#L350 - we have access to the optional action to call.

It could then be called after the OnIntercepted processing but before the response is built. At this time it's been found, any optional intercept shenanigans has happened and we're about to return the response - https://github.com/justeat/httpclient-interception/blob/main/src/HttpClientInterception/HttpClientInterceptorOptions.cs#L360-L362

This way the API surface is clean. There is no reliance on the id/name of the connection. It is an easy customisable construct which can be used and will be backwards compatible.

Will try and find some time soon to put together a proper POC to discuss further but in the mean time let me know your thoughts on this approach.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[drewburlingame]

We could really use this feature. Have there been any updates on it? Is there anything we could help with?

@WestDiscGolf can you share an example of your work around?

[WestDiscGolf]

@drewburlingame If I recall the work around was using the Content Predicate functionality and either having an additional Func<> which is called and/or setting a variable inside that which could be referenced outside of the scope and check it had been set. I'm afraid I don't work at the same place anymore to refresh my memory and haven't used the library in a while. Sorry 😃

[drewburlingame]

Thanks for coming back to this @WestDiscGolf. We were able to use the extension HttpRequestInterceptionBuilder.WithInterceptionCallback to set a variable and capture context from the request message, similar to what you described. Cheers.