-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support for anyNumberOf expectations (fixes #18) #19
Support for anyNumberOf expectations (fixes #18) #19
Conversation
jazon-core/src/main/java/com/zendesk/jazon/expectation/Expectations.java
Show resolved
Hide resolved
@@ -158,4 +160,17 @@ class MatcherForGroovySpec extends Specification { | |||
{ it.color == 'red' }, | |||
] | |||
} | |||
|
|||
def "array each element expectation works correctly with lambda-style expectation"() { |
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.
Wan't sure if this test is strictly required, adding it for good measure
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 looks great :) I have mentioned some minors that I find worth fixing. Please consider :) Do not hesitate to disagree if you see that I am wrong.
import lombok.RequiredArgsConstructor; | ||
|
||
@RequiredArgsConstructor | ||
public class AnyNumberOf { |
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.
It is not a big deal but - as long as it can be - let's make it package-private, not public.
@RequiredArgsConstructor | ||
public class AnyNumberOf { | ||
|
||
@Getter |
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 getter best made package private too
@Override | ||
public MatchResult match(ActualJsonArray actualArray, String path) { | ||
List<Actual> actualValues = actualArray.list(); | ||
for (int i = 0; i < actualValues.size(); ++i) { |
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 may not be a big deal but generally it would be more safe to iterate using an Iterator
interface. This is because List.get()
does not guarantee constant time complexity. I would err on the side of caution here - just to be sure this will never be the reason to fix this part of code.
result.mismatch().expectationMismatch() == elementMismatch | ||
result.mismatch().path() == '$.a.' + elementIndex | ||
|
||
where: |
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.
It would be great to add 2 additional cases:
- for sample json-object as the repeated member of verified array
- for sample json-array as the repeated member of verified array
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.
Added tests for maps and lists as the expected element. Not sure if that's what you had in mind
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 one is cool :)
|
||
public class Expectations { | ||
|
||
public static AnyNumberOf anyNumberOf(Object element) { |
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.
👍
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.
Looks good now
result.mismatch().expectationMismatch() == elementMismatch | ||
result.mismatch().path() == '$.a.' + elementIndex | ||
|
||
where: |
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 one is cool :)
I am gonna merge this and release a new version later this week. |
No description provided.