-
Notifications
You must be signed in to change notification settings - Fork 233
feat: add array contains matcher #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: add array contains matcher #518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FFI change should probably be split into a separate preceding change just because of the extra complexity around the breaking changes in the FFI packaging. I could see that causing issues so a dedicated change would be better.
In terms of the example, the pact spec describes this matcher as checking that all variants are contained within a collection, but this example doesn't really reflect that given the collection is only of one type and thus only one variant. The eachLike matcher is better in that scenario. I think the example should somehow contain different types (and if it's too difficult to clearly do that then perhaps the matcher doesn't make as much sense in a typed language Vs something like JavaScript).
I've not reviewed the integration tests as I'm on my phone and they're quite big, so there may be further comments there later.
Also check any line endings settings in your editor because a few places have weird extra blank lines and things, plus obviously the sln file shows a huge diff when there probably isn't one.
PactNet.sln
Outdated
SolutionGuid = {C2CBC30C-92D4-4E3A-A5B8-1E5D4E938DFC} | ||
EndGlobalSection | ||
EndGlobal | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Something appears to have gone weird with this file, perhaps line endings? Iirc sln files must have CRLF line endings always, even on non-Windows systems
The gitignore file also may have this issue because the final line shows a change when there isn't one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note sure what is causing this one, think it may be my local git settings. Seemingly have the right CRLF settings, but assuming the issue is something similar
"combine": "AND", | ||
"matchers": [ | ||
{ | ||
"match": "type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This matching rule looks wrong, should it be a date matcher instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from this line:
Date = Match.Type(expected1.Date.ToString("O"))
So is indeed meant to be type
@@ -0,0 +1,17 @@ | |||
using System.Text.Json.Serialization; | |||
|
|||
namespace PactNet.Matchers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The project uses namespace blocks instead of file scoped namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will amend
|
||
namespace PactNet.Matchers; | ||
|
||
public class ArrayContainsMatcher : IMatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: The public API should be documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will amend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to go through this, especially on a weekend.
I will open a new MR to just update the FFI, and will come back to this one to make the requested changes
"combine": "AND", | ||
"matchers": [ | ||
{ | ||
"match": "type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from this line:
Date = Match.Type(expected1.Date.ToString("O"))
So is indeed meant to be type
@@ -0,0 +1,17 @@ | |||
using System.Text.Json.Serialization; | |||
|
|||
namespace PactNet.Matchers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will amend
|
||
namespace PactNet.Matchers; | ||
|
||
public class ArrayContainsMatcher : IMatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will amend
@MaxCampman if you rebase onto latest then the FFI upgrade should already be there 👍 |
1915b96
to
ff6f37c
Compare
ff6f37c
to
b03a9ea
Compare
.WithStatus(HttpStatusCode.OK) | ||
.WithJsonBody(Match.ArrayContains(new dynamic[] | ||
{ | ||
new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: I think for this example to 'come to life' then we'd really need it to match multiple different possible variants
Perhaps if there were different order types or something so that they could have different fields on them, then the matching rules could reflect those variants. Like a fulfilled order has an extra date on it for when it was completed, or something like that.
.WithStatus(HttpStatusCode.OK) | ||
.WithJsonBody(Match.ArrayContains([ | ||
Match.Regex("Thing 1", "Thing [0-9]+"), | ||
Match.Regex("Thing X", "Thing [A-Z]+"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: This is a good test because it shows using different variants, which we should replicate in the example usage
/// Initialises a new instance of the <see cref="ArrayContainsMatcher"/> class. | ||
/// </summary> | ||
/// <param name="variants"></param> | ||
public ArrayContainsMatcher(dynamic[] variants) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: This passes in an array, but we store the Value
field as a single dynamic
- is that correct? I would've thought it would have been a collection also.
suggestion: Use ICollection
instead of an array for a more ergonomic API
var strings = await service.GetStringList(); | ||
|
||
// Assert | ||
strings.Should().Contain("Thing 1").And.Contain("Thing 2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use Should().Equal(["Thing 1", "Thing 2"]}
to be more explicit that it should contain all of the expected, and only the expected
What?
src/PactNet.Abstractions/Matchers/ArrayContainsMatcher.cs
src/PactNet.Abstractions/Matchers/Match.cs
src/PactNet.Abstractions/Matchers/MatcherConverter.cs
NOTE:
if [[ "$OSTYPE" == "darwin"* ]]; then
)