feat: Remote mock server service for consumer tests

[HugoDoyon]

Add the possibility to use a remote mock server service for the consumer tests.

For instance a remote mock server could be call to a pact-cli docker container.

To run your consumer tests against a remote server, in PactBuilder class, set the "useRemoteMockService" parameter to true.

See readme.md for more details

Fix the following issues:

• Remote Mock Server Issue #245
• Build failing for PactNet.Windows Ruby file Path being too long. Issue #163

[bethesque]

I can't comment on the code changes, but the concept is 👍🏽

[mefellows]

What Beth says - cool!

[EtienneLorthoy]

I would love to see this in master soon !

[charlesericjs]

This addition would be very useful! Changes look legit to me.

[bethesque]

I think Neil is a bit busy to review/release right now. Is there anyone else who can do it?

[jacekmlynek]

Hi,

I have some thoughts related to code changes can comment on it later on, but that is probably all. It doesn't look like I can merge this to master.

JM.

[jacekmlynek]

As I have promised I have left few comments/suggestions. Sorry that it took me a while.

Please let me know if something will not be clear.

[mefellows]

Thanks for the collaboration on this folks 👏

Are either of you keen to help be a co-maintainer of the project? Let us know, we'd love to support you!

[HugoDoyon]

Hi @mefellows, it will be my great honor to join you as co-maintainer as soon as this PR is merged, Thank you! 😃

[jacekmlynek]

Hi @mefellows I am also honored with your offer to become co-maintainer. I do not always have so much time I wish to have, but I am keen to help . Thanks JM.

[jacekmlynek]

Overall code looks good to me, just one more small suggestion.

[mefellows]

Awesome work folks! When you're ready to release this thing, let us know (and maybe also ping us in #pact-net in the slack workspace (join|visit) and we can work through the details.

[jacekmlynek]

Since you have used using static System.String;, you may also consider using it in terms of IsNullOrEmpty

[jacekmlynek]

@HugoDoyon look good to me - really small suggestion from my point of view it can be merged.
@mefellows I am already in the slack.

[martincostello]

While investigating why an update to 2.6.2 via dependabot has broken a project of mine, reviewing the diff between 2.6.1 and 2.6.2 shows that this PR introduced binary and source-breaking compatibility issues by adding new members to the public IPactBuilder and IMockProviderService interfaces.

This change should have ideally been released as a v3.0.0 to comply with SemVer, been done in a way that didn't cause binary breaking compatibility, or at least have been released as a minor version bump to 2.7 if the major version number is being kept in sync with Pact itself.

I'm still trying to work out what change has actually broken my CI - will open an issue if I get to the bottom if it.

[mefellows]

Thanks Martin, appreciate you picking this up, apologies for any issue.

I think the best course of action is to disable new installs of the new package (via Nuget), and re-release as a 3.0.0. It doesn't really feel like a 3.0.0 release in nature, but the interface compatibility issue isn't ideal.

@HugoDoyon @jacekmlynek - thoughts?

FWIW: Most of the other Pact projects use conventional commits and automated changelogs - something I'd like to do on this now that I'm helping out with the release.

[jacekmlynek]

@mefellows It is new feature so at least it should be released with minor version bump by default. Now, I can agree that changes in this PR could break someone else code if he is implementing one of mention interfaces, which make it a good reason to bum major version (I think we must catch this earlier - maybe using gitversion will help). However this looks for me like static source incompatibility change.

Less important, but maybe worth to touch a bit is mentioned by @martincostello breaking binary compatibility. I just would like to understand how in your case it breaks binary compatibility? Do you not compile your code before publishing it? Furthermore I am not sure if there is anything about binary compatibility in semver. I think semantic versioning leave wiggle-room in this case. Saying that I am aware that there were some discussion about additional indicator, but I am not sure how it is ended. Nerveless, it will be good to understand your problem a bit more for the future releases.

[martincostello]

It breaks binary compatibility by amending the public interfaces. Any libraries that maybe extend or consume Pact in some way and are distributed further would be broken due to the interfaces being changed.

Adding optional parameters is a binary-breaking change as the method signature changes from a runtime perspective, even if it isn't neccessarily a source-breaking change. Adding the additional property to IMockProviderService is source and binary breaking because existing implementations won't have the UseRemoteMockService property implemented.

In my case it didn't break the source as I don't do that in my projects, but from a C#/.NET perspective, these changes are breaking to anyone who may have done that with Pact.

[mefellows]

Thanks folks. So to remediate this and prevent further frustrations for others, shall I re-release under 3.0.0 and deprecate the 2.6.2 version?

[martincostello]

I'm still trying to work out what change has actually broken my CI - will open an issue if I get to the bottom if it.

Regarding this, I'm experiencing timeouts in some tests I have where the test process seems to be deadlocking. From viewing the diff further, I think the problem has been introduced by the change at the bottom of this file: https://github.com/pact-foundation/pact-net/compare/2.6.1..2.6.2#diff-8ea699804b2cb88e7361e4f10ad670345b83a1d891e980848c80994d6c535104

SendAdminHttpRequest() is an async method that returns a Task<string> - in version 2.6.1 it wasn't awaited, so didn't cause too much of an issue. However now with the result being used, it's not awaited and might now have caused some subtle timing change which is causing xunit to deadlock/hang.

pact-net/PactNet/PactBuilder.cs

Line 128 in ffa6ab8

var responsePact = _mockProviderService.SendAdminHttpRequest(HttpVerb.Post, Constants.PactPath);

Should the call be wrapped in Async.RunSync() like some of the other calls in the diff like here?

pact-net/PactNet/Mocks/MockHttpService/MockProviderService.cs

Line 171 in ffa6ab8

Async.RunSync(() => _adminHttpClient.SendAdminHttpRequest(HttpVerb.Delete, $"{Constants.InteractionsPath}?"));

I think the most likely culprit is .Result being used here:

pact-net/PactNet/Mocks/MockHttpService/MockProviderService.cs

Line 132 in 572f04f

return _adminHttpClient.SendAdminHttpRequest(method, path, headers:headers).Result;

[jacekmlynek]

@martincostello as far as I understood binary incompatibility was not your case, you just share some generic thought. Is this right?

@mefellows that sound right to me, but I think it is far to wait for @HugoDoyon as well.

[martincostello]

@martincostello as far as I understood binary incompatibility was not your case, you just share some generic thought. Is this right?

Yep that's right, my issue is a deadlock. I was just mentioning it from a general principal standpoint. It was investigating what might be causing the deadlock that lead me to spot the changes to the interfaces.

[mefellows]

@mefellows that sound right to me, but I think it is far to wait for @HugoDoyon as well.

NP. I've just unlisted the package until we sort this out to err on the side of caution. I'll be offline probably (getting late here) so will pick any actions up in the morning.

[jacekmlynek]

I am not a big expert of async Task in c#, but I took a quick look on the code and unfortunately it looks like a classic example of possible deadlock:

1. MockProviderService calling SendAdminHttpRequest which is an async method
2. SendAdminHttpRequest starts http request by calling _httpClient.SendAsync
3. SendAdminHttpRequest awaits _httpClient.SendAsync and will continue running later.
4. MockProviderService synchronously blocks on the Task returned by SendAdminHttpRequest, which blocks context thread
5. At some point http request task is ready and want continue, but must wait for context thread, which has been block by calling Result - deadlock

In short term I think we could add to Async helper class something like this:

public static TResult RunSync<TResult>(Func<Task<TResult>> task)
           => _taskFactory
                .StartNew(task)
                .Unwrap()
                .GetAwaiter()
                .GetResult();

And used this instead of Result

In long term we should redesign contract behind SendAdminHttpRequest IMHO it should not expose asyn methods, unless we will not want to expose public async method too. Hope it is clear. I am just not sure how can reproduce @martincostello problem and test purpose fix.

@HugoDoyon sorry that I didn't catch that in code review.

[mefellows]

Thanks for digging in Jacek. I'm out of my depth here (no C#/.NET knowledge) so will provide a supporting role. It would be great to be able to repro, but also, it sounds like the async call is mishandled anyway.

@martincostello if the patch described above was added to this PR, could you test if the deadlock occurs?

[martincostello]

I'll try and repro and then check for a fix with a patch some time today.

[martincostello]

I'll try and repro and then check for a fix with a patch some time today.

#293