# System-1: System testing: Deviation added #3416
Conversation
Pull Request Functional Test Report for #3416 / cbfcba4Virtual Devices
Hardware Devices
|
Pull Request Test Coverage Report for Build 11586703328Details
💛 - Coveralls |
2e9b545 to
38706dc
Compare
0850645 to
dcbc957
Compare
| @@ -58,8 +59,14 @@ func TestGNMIClient(t *testing.T) { | |||
| dut := ondatra.DUT(t, "dut") | |||
| conn := dialConn(t, dut, introspect.GNMI, 9339) | |||
| c := gpb.NewGNMIClient(conn) | |||
| if _, err := c.Get(context.Background(), &gpb.GetRequest{Encoding: gpb.Encoding_JSON_IETF, Path: []*gpb.Path{{Elem: []*gpb.PathElem{}}}}); err != nil { | |||
| t.Fatalf("gnmi.Get failed: %v", err) | |||
| if deviations.GnmiGetRequiresConfigType(dut) { | |||
There was a problem hiding this comment.
Why is this a deviation and/or required? What is the behavioural difference that is driving this change?
The gNMI protobuf has ALL as the default data type. We should not accept different Get implementations that try and impose mandatory defaults if this does not meet the requirements. Please review this change and determine what it is we are actually trying to cover rather than making changes that simply make the test pass.
@xw-g for visibility. @sezhang2 -- because this looks like a Juniper change that is being made.
I'm marking this as 'requires changes' to block submission until this has been reviewed further. Please ping me directly once we've provided adequate justification here.
There was a problem hiding this comment.
Hi robshakie,
As per the comment suggested by @dplore this change was made with the deviation request-
#3265 (comment)
Please let me know if any other changes needs to be done.
Thanks,
C Akhil
There was a problem hiding this comment.
@robshakir If seems Junos only supports gNMI.Get on config leaves. This is not compliant with gnmi, but we also want to be aware of this limitation.
I recommend that we accept a deviation for this because this is not blocking operational requirements and allows us to document the issue. Juniper should remove this deviation on a future release of Junos.
Note that IOSXR has a related but different issue; gNMI.Subscribe is not supported on config leaves.
There was a problem hiding this comment.
@robshakir gnmi get for config path now requires type=config, it was already part of the gnmi spec(https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto#L409). gnmi get rpc for state is redundant as subscribe with once mode achieves the same. Kindly review further and let us if this is acceptable
There was a problem hiding this comment.
What is our plan to resolve this deviation? Please ping me internally with this information. I think this is proliferating an ambiguity in requirements. No change to my blocking comments.
| @@ -58,8 +59,14 @@ func TestGNMIClient(t *testing.T) { | |||
| dut := ondatra.DUT(t, "dut") | |||
| conn := dialConn(t, dut, introspect.GNMI, 9339) | |||
| c := gpb.NewGNMIClient(conn) | |||
| if _, err := c.Get(context.Background(), &gpb.GetRequest{Encoding: gpb.Encoding_JSON_IETF, Path: []*gpb.Path{{Elem: []*gpb.PathElem{}}}}); err != nil { | |||
| t.Fatalf("gnmi.Get failed: %v", err) | |||
| if deviations.GnmiGetRequiresConfigType(dut) { | |||
There was a problem hiding this comment.
@robshakir gnmi get for config path now requires type=config, it was already part of the gnmi spec(https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto#L409). gnmi get rpc for state is redundant as subscribe with once mode achieves the same. Kindly review further and let us if this is acceptable
64a6b9d to
5f46de4
Compare
71c9229 to
63a162a
Compare
|
/fptest virtual |
as per darrens comment in the test disimissing the review
As per comment given in the pull #3265 , Added deviation.