Generate custom tags for oneof fileds (#611) #612
Conversation
krhubert
changed the title
|
Hi. Thanks for the pullrequest. It seems like the oneof tags were collateral damage of a large generator refactor. |
|
Can we please also get a test for this issue? I think it would make sense to add a simple oneof message under" |
|
@jmarais I think it's ready to review. In tests I skip the Marshal for json and XML because I will have to implement custom marshaler for oneof interface. |
There was a problem hiding this comment.
The generator change looks good.
Do you have a suggestion for a test in order to validate a custom moretags field?
I will feel more confident if we can get a test in order to validate this behavior in order to prevent it reoccurring in the future.
With regards to the travis job. Remember to make in the root of the project in order to regenerate all the protos with the generator change.
test/tags/tags_test.go
Outdated
| @@ -86,29 +90,34 @@ func TestJson(t *testing.T) { | |||
| if msg2.MyField2 != msg1.MyField2 { | |||
| t.Fatalf("proto field2 %s != %s", msg2.MyField2, msg1.MyField2) | |||
| } | |||
| if msg2.MyField3 != msg2.MyField3 { | |||
There was a problem hiding this comment.
I do not think this test works. We will have to check field3 after the first Outside unmarshal above. If I add that check the intermittently fail because it is just dropping MyField3. Currently, this last check doesn't catch anything becuase with the second marshal/unmarshal MyField3 always stays empty.
test/tags/tags_test.go
Outdated
| msg2 := &Outside{ | ||
| Inside: &Inside{ | ||
| Field1: &field1, | ||
| }, | ||
| Field2: &field2, | ||
| Filed: &Outside_Field3{ | ||
| Field3: field3, | ||
| }, | ||
| } | ||
| if msg1.GetField1() != msg2.GetField1() { | ||
| t.Fatalf("field1 expected %s got %s", msg2.GetField1(), msg1.GetField1()) | ||
| } |
There was a problem hiding this comment.
Field3Value was added to the starting data, and added to msg2, however we are not validating that field3 match the expected value after the unmarshal.
We would need an additional:
if msg1.GetField3() != msg2.GetField3() {
t.Fatalf("field3 expected %s got %s", msg2.GetField3(), msg1.GetField3())
}check. If I add this check the test does fail.
|
About comments for tests look on the answer below.
What do you think about using reflection and get a tag, instead of marshal/unmarshal with json/xml?
Yes, I miss that. Also I found something interesting - in tags.proto syntax is set to proto2, but the generated |
That is a good suggestion. I think that will make for a nice test.
That is just a constant generated by the |
- add test case in tags.proto file
|
@jmarais I regenerated proto definitions plus replace the test to make use of reflection. |
No description provided.