https://pact.io logo
Join Slack
Powered by
Hi everyone. Following on from a message on <iss...
# pact-js
d
Hi everyone. Following on from a message on issue 304. I've polished up the work that @Matt (pactflow.io / pact-js / pact-go) had started to be able to change the request body in the verifier.requestFilter(). I've never written unit tests for middleware before and there is not much out there for writing tests using http-proxy, so this is a first attempt, if anyone is available to just check this seems ok, before I raise the p/r. https://github.com/DaveClissold/pact-js/tree/feat/modify-req.body-in-requestfilter-v2. If everyone seems happy with this, I'll add these tests to v3 and raise a pr for that as well
🙌 1
m
I can definitely review
My suggestion would be to move as much out of the HTTP proxy layer into (where possible) pure JS functions. We can test those separately then.
What remains could then be tested either via an integration test of 
http-proxy
(which, yes, will probably be a bit difficult/messy and maybe not worth it - see how you go) or via a end to end test by adding functionality to the v3 e2e example (these run with every build, so it will catch a regression)
d
Thanks Matt, I was having a headache with finding the right class to stub with sinon but after digging through the the type defs and multiple extended classes have a working solution. Will raise the pr now for v2 and work on the v3 one a bit later
My day got away from me .. here is the pr https://github.com/pact-foundation/pact-js/pull/873
m
thanks! I’ll take a look over the next few days
Much appreciated!!
d
Hi @Matt (pactflow.io / pact-js / pact-go) Just noticed the failed test runs. Have fixed the problem and updated the pr
Here is a pr for the v3 branch as well https://github.com/pact-foundation/pact-js/pull/875
m
Awesome, thanks! I didn’t get time for a proper review, but realised I needed to unblock the run
thanks for fixing
it looks small enough of a change we should be able to pull it in. I’ll compare to the branch as well and try and jig the memory - I’m assuming you’ve tested actually using it locally also?
d
Hey Matt, apologies, spotted I missed pushing a commit on the v3 branch 🤦‍♂️. Yeah have tested everything locally. Not sure how to resolve the failures on the v2 branch, that looks like it's something within your pipe. Am happy to upload screen capture evidence of local runs and attach them to the pr if it helps?
👍 1
m
All good, I thought they looked a bit light on 🤣
I'll address the pipe issues, early next week should be able to properly review and merge
Really appreciate it
d
No, thank you for helping get this in, we need it to implement pact into our testing. Once I can prove to the boss I've got it running in the pipeline on one service, I can get those magic 16 digits for a full pactflow account.
m
OK that was a bit of a pain. For some reason we’ve started seeing some new failures that are definitely unrelated to Pact JS code changes. In any case, a cleanup was required and has been done, and your changes are now released in the latest version: 9.18.0. I seem to have an issue with it in the V3 branch, so i’ll have to dive into it. Provider side tests seem to be hanging, and reverting the proxy change. I’ll take a look this week, or if I’m lucky tonight
Ok found the issue. I've released the v3 branch too. Let me know how it goes!
d
Thanks Matt, apologies for the delay in replying, had a few days off. Everything is looking good in both branches
🙌 1
m
Great thanks! Really appreciated your help 🙏
3 Views
Open in Slack
PreviousNext
Powered by