-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for repeated fields in field mask tests #250
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
Conversation
under Windows based systems.
Codecov Report
@@ Coverage Diff @@
## master #250 +/- ##
============================================
+ Coverage 66.38% 66.43% +0.04%
- Complexity 583 584 +1
============================================
Files 310 310
Lines 8158 8167 +9
Branches 561 562 +1
============================================
+ Hits 5416 5426 +10
+ Misses 2625 2624 -1
Partials 117 117 |
@armiol, PTAL. |
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.
@serhii-lekariev please see my comments.
base/src/main/java/io/spine/code/proto/RejectionDeclaration.java
Outdated
Show resolved
Hide resolved
assertThat(fieldNames).containsAllIn(paths); | ||
|
||
// Assert that values match the field mask. | ||
for (FieldDescriptor field : fields) { |
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 piece of code looks unformatted.
for (FieldDescriptor field : fields) { | ||
if (field.isRepeated()) { | ||
continue; | ||
boolean pathsHasSuchField = paths.contains(field.getName()); |
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.
Let's discuss what this test does.
@armiol, PTAL. |
@dmdashenkov, PTAL. |
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.
@serhii-lekariev, please see my comments.
@dmdashenkov, PTAL. |
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.
@serhii-lekariev, please fix a few minor comments and LGTM.
This PR addresses #165.
Now
assertMatchesMask
doesn't skip repeated fields, and instead checks whether at least 1 value is present, and calls a respectiveassert
.However, repeated fields are only allowed as a last field in the respective mask, so while mask
can match for a user with friends
a mask
cannot match for any user, because a repeated field is not used as a last one.