pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.61k stars 344 forks source link

fix reify logic relating to isMatcher function #970

Closed hborham closed 1 year ago

hborham commented 1 year ago

isMatcher inadvertantly passes when a field name is 'value'

Thank you for making a pull request!

Pact-JS is built and maintained by developers like you, and we appreciate contributions very much. You are awesome!

Here is a short checklist to give your PR the best start:

Everything above can be removed once checked

Commit messages

Our changelog is automatically built from our commit history, using conventional changelog. This means we'd like to take care that:

If you've made many commits that don't adhere to this style, we recommend squashing your commits to a new branch before making a PR. Alternatively, we can do a squash merge, but you'll lose attribution for your change.

For more information please see CONTRIBUTING.md

Everything above can be removed

PR Template

Please describe what this PR is for, or link the issue that this PR fixes

You may add as much or as little context as you like here, whatever you think is right

Thanks again!

hborham commented 1 year ago

I suspect there's a preferred way to enhance the isMatcher to not be true for just AnyTemplate with a value field. Please feel free to suggest and/or modify this PR

TimothyJones commented 1 year ago

This is a great catch. I don't know why I didn't write it that way to start with - your way is definitely both more accurate and better.

I'm not a maintainer any more so I can't enable the workflow - but it would be great if you could push a different commit message that adheres to the guidelines - it needs a : after fix so that it gets picked up by the tooling, and it needs to be a message that makes sense in the changelog. Something like fix: Fix an issue where extractPayload would not work correctly with objects with a value key, or whatever you think makes sense.

Thanks for covering it with a test too! Much appreciated!

mefellows commented 1 year ago

Thanks, and good pickup!

Looks like you've been hit by the formatting checker - you should be able to fix this with an npm run format:fix.

I've noticed recently the Windows build failing for something to do with native dependency compilation, if the Linux builds pass I'll merge.

hborham commented 1 year ago

Thanks - just amended.

mefellows commented 1 year ago

Agreed 🙏