[1/4] Compliance test fixes. (#80)

* Add ccli parameters, update expected error response codes.

  * (M) client/client.go
    - Avoid panic when details are not populated.
  * (M) cmd/ccli/ccli_test.go
    - Add command line flag for skipping tests based on requiring FIB
      FIB ACK to allow skipping these tests where this is unimplemented.
  * (M) compliance/compliance.go
    - Add support for a caller to specify the election ID to be used.

* Send non-empty NH, improve client error output.

  * (M) client/client.go
    - Add String() method for operation result's details such that there
      is additional information as to what the error corresponds to.
  * (M) compliance/compliance.go
    - Ensure that we do not send an empty next-hop when at least one
      field must be populated. See #79 which tracks adding a fix to the
      server side code.

* Add support for allowing unimplemented in a check.

  * (M) chk/chk.go
    - Add options for checking errors that can be used to check whether
      something is unimplemented OR a specific error.
  * (M) compliance/compliance.go
    - Adopt this check for failed precondition vs. unimplemented for
    ALL_PRIMARY.

* [2/4] Bug fixes in test code. (#81)

* Bug fixes in test code.

  * (M) chk/chk.go
    - Add better error message formatting for debugging.
  * (M) client/client.go
    - Add a special case for a match used in compliance tests where the
      operation ID expected is unspecified.
  * (M) cmd/ccli/ccli_test.go
    - Rename some flags for consistency, allow tests that require
      reordering of transactions by the server to be skipped.
  * (M) compliance/compliance.go
    - Restructure the compliance tests to have a generator function that
      can be used to specify the ACK type, rather than needing to write
      wrappers manually.

* Specify InstalledInRIB as an argument.

  * (M) demo/ipv4/ipv4_test.go
  * (M) device/device_test.go
    - Adopt renamed tests.

* [3/4] Add compliance tests for metadata, different NI NHG. (#68)

* Add compliance tests for IPv4 entries, fix server defect.

  * (M) client/client.go
    - Improve logging.
  * (M) compliance/compliance.go
    - Add test for adding IPv4 entry in a single ModifyRequest,
     and within separate requests.
  * (M) fluent/fluent.go
    - Add support for WithIPAddress to builder.
  * (M) gnmit/gnmit.go
    - Minor refactor to use a new GNMIServer container - no-op
    change for adding gNMI Set support.
  * (M) rib/rib.go
    - Improve logging.
  * (M) server/server.go
  * (M) server/server_test.go
    - Resolve a defect whereby the server called modifyEntry within
      a goroutine per operation within a ModifyRequest. In the case
      where a single ModifyRequest made multiple entries resolvable,
      then an entry could be duplicate ACK'd, by all the entries that
      made it resolvable. Whilst the RIB is safe to modify in this
      manner, the pending queue could result in this occurring - the
      duplicate install is a NOOP, but the duplicate ACK violated the
      API contract. Initially, resolve this issue by serially processing
      modify requests - added a TODO to cover a less naive solution.

* Whitespace fixes.

* Add compliance tests for deleting entries.

  * (M) client/client.go
    - Improve debugging output of an operation result.
  * (M) compliance/compliance.go
    - Add tests for deleting IPv4 entries, NHGs, NHs - both positive
      and negative tests.
  * (M) fluent/fluent.go
    - Add ability to reference a NHG NI from an IPv4Entry.
  * (M) go.sum
    - Tidy up go modules.

* Fix tests to adopt new client stub argument.

* Add compliance tests for metadata, different NI NHG.

 * (M) client/client.go
  - Improve string message for OpResults.
 * (M) compliance/compliance.go
  - Add IPv4Entry -> NHG in different NI test - currently skipped
    based on server configuration.
  - Add test for metadata on IPv4Entry.
 * (M) fluent/fluent.go
  - Add fluent function for adding metadata.

* Fix support for comparing to unimplemented.

  * (M) chk/chk.go
    - Ensure that details are maintained when checking for
    unimplemented.
  * (M) compliance/compliance.go
    - Clean up compliance test.
  * (M) go.mod
  * (M) go.sum
    - Add golang gRPC genproto dependency.

* Update test names.

  * (M) demo/ipv4/ipv4_test.go
  * (M) device/device_test.go
    - Specify RIB ACK as an argument rather than using the removed test.

* [4/4] Handle nil responses when debugging. (#73)

* Do not send election_id = 0.

  * (M) compliance/compliance.go
    - Ensure that the compliance tests do not send election_id = 0
      which is an invalid value.

* Add server handling for election ID = 0.

  * (M) server/server.go
  * (M) server/server_test.go
    - Ensure that election ID set to zero is responded to with an
      error.

* Handle nil responses when debugging.

  * (M) client/client.go
  * (M) client/client_test.go
    - Ensure that a nil input value is cleanly handled when calling
      String() on a nil OpResult.

* Fix string error reporting for partially specified messages.

  * (M) client/client.go
  * (M) client/client_test.go
    - Fix a panic in OpResult.String that could occur for
      partially populated messages.

chk/chk.go

@@ -21,14 +21,19 @@
 package chk
 
 import (
+	"bytes"
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/openconfig/gribigo/client"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
+
+	spb "google.golang.org/genproto/googleapis/rpc/status"
 )
 
 // resultOpt is an interface implemented by all options that can be
@@ -91,7 +96,13 @@ func HasResult(t testing.TB, res []*client.OpResult, want *client.OpResult, opt
 		}
 	}
 	if !found {
-		t.Fatalf("results did not contain a result of value %s, got: %v", want, res)
+		buf := &bytes.Buffer{}
+		buf.WriteString(fmt.Sprintf("results did not contain a result of value %s\n", want))
+		buf.WriteString("got:\n")
+		for _, r := range res {
+			buf.WriteString(fmt.Sprintf("\t%s\n", r))
+		}
+		t.Fatalf(buf.String())
 	}
 }
 
@@ -123,22 +134,54 @@ func HasNRecvErrors(t testing.TB, err error, count int) {
 	}
 }
 
+// ErrorOpt is an interface that is implemented by functions that examine errrors.
+type ErrorOpt interface {
+	isErrorOpt()
+}
+
+// allowUnimplemented is the internal representation of an ErrorOpt that allows for
+// an error that is unimplemented or a specific error.
+type allowUnimplemented struct{}
+
+// isErrorOpt marks allowUnimplemented as an ErrorOpt.
+func (*allowUnimplemented) isErrorOpt() {}
+
+// AllowUnimplemented specifies that receive error with a particular status can be unimplemented
+// OR the specified error type. It can be used to not return a fatal error when a server does
+// not support a particular functionality.
+func AllowUnimplemented() *allowUnimplemented {
+	return &allowUnimplemented{}
+}
+
 // HasRecvClientErrorWithStatus checks whether the supplied ClientErr ce contains a status with
 // the code and details set to the values supplied in want.
-func HasRecvClientErrorWithStatus(t testing.TB, err error, want *status.Status) {
+func HasRecvClientErrorWithStatus(t testing.TB, err error, want *status.Status, opts ...ErrorOpt) {
 	t.Helper()
 
+	okMsgs := []*status.Status{want}
+	for _, o := range opts {
+		if _, ok := o.(*allowUnimplemented); ok {
+			uProto := proto.Clone(want.Proto()).(*spb.Status)
+			uProto.Code = int32(codes.Unimplemented)
+			unimpl := status.FromProto(uProto)
+			okMsgs = append(okMsgs, unimpl)
+		}
+	}
+
 	var found bool
 	ce := clientError(t, err)
 	for _, e := range ce.Recv {
-		s, ok := status.FromError(e)
-		if !ok {
-			continue
-		}
-		ns := s.Proto()
-		ns.Message = "" // blank out message so that we don't compare it.
-		if proto.Equal(ns, want.Proto()) {
-			found = true
+		for _, wo := range okMsgs {
+			s, ok := status.FromError(e)
+			if !ok {
+				continue
+			}
+			ns := s.Proto()
+			ns.Message = "" // blank out message so that we don't compare it.
+
+			if proto.Equal(ns, wo.Proto()) {
+				found = true
+			}
 		}
 	}
 	if !found {

client/client.go

@@ -518,22 +518,12 @@ type OpResult struct {
 	Details *OpDetailsResults
 }
 
-// OpDetailsResults provides details of an operation for use in the results.
-type OpDetailsResults struct {
-	// Type is the type of the operation (i.e., ADD, MODIFY, DELETE)
-	Type constants.OpType
-
-	// NextHopIndex is the identifier for a next-hop modified by the operation.
-	NextHopIndex uint64
-	// NextHopGroupID is the identifier for a next-hop-group modified by the
-	// operation.
-	NextHopGroupID uint64
-	// IPv4Prefix is the IPv4 prefix modified by the operation.
-	IPv4Prefix string
-}
-
 // String returns a string for an OpResult for debugging purposes.
 func (o *OpResult) String() string {
+	if o == nil {
+		return "<nil>"
+	}
+
 	buf := &bytes.Buffer{}
 	buf.WriteString("<")
 	buf.WriteString(fmt.Sprintf("%d (%d nsec):", o.Timestamp, o.Latency))
@@ -544,7 +534,14 @@ func (o *OpResult) String() string {
 	}
 
 	if v := o.OperationID; v != 0 {
-		buf.WriteString(fmt.Sprintf(" AFTOperation { ID: %d, Type: %s, Status: %s }", v, o.Details.Type, o.ProgrammingResult))
+		typ := "Unknown"
+		if o.Details != nil {
+			typ = o.Details.Type.String()
+		}
+		buf.WriteString(fmt.Sprintf(" AFTOperation { ID: %d, Type: %s, Status: %s }", v, typ, o.ProgrammingResult))
+	} else if v := o.ProgrammingResult; v != spb.AFTResult_UNSET {
+		// Special case for input messages that are just matching on status.
+		buf.WriteString(fmt.Sprintf(" AFTOperation { Status: %s }", v))
 	}
 
 	if v := o.SessionParameters; v != nil {
@@ -559,6 +556,40 @@ func (o *OpResult) String() string {
 	return buf.String()
 }
 
+// OpDetailsResults provides details of an operation for use in the results.
+type OpDetailsResults struct {
+	// Type is the type of the operation (i.e., ADD, MODIFY, DELETE)
+	Type constants.OpType
+
+	// NextHopIndex is the identifier for a next-hop modified by the operation.
+	NextHopIndex uint64
+	// NextHopGroupID is the identifier for a next-hop-group modified by the
+	// operation.
+	NextHopGroupID uint64
+	// IPv4Prefix is the IPv4 prefix modified by the operation.
+	IPv4Prefix string
+}
+
+// String returns a human-readable form of the OpDetailsResults
+func (o *OpDetailsResults) String() string {
+	if o == nil {
+		return "<nil>"
+	}
+	buf := &bytes.Buffer{}
+	buf.WriteString(fmt.Sprintf("<Type: %s ", o.Type))
+	switch {
+	case o.NextHopIndex != 0:
+		buf.WriteString(fmt.Sprintf("NH Index: %d", o.NextHopIndex))
+	case o.NextHopGroupID != 0:
+		buf.WriteString(fmt.Sprintf("NHG ID: %d", o.NextHopGroupID))
+	case o.IPv4Prefix != "":
+		buf.WriteString(fmt.Sprintf("IPv4: %s", o.IPv4Prefix))
+	}
+	buf.WriteString(">")
+
+	return buf.String()
+}
+
 // Q enqueues a ModifyRequest to be sent to the target.
 func (c *Client) Q(m *spb.ModifyRequest) {
 	if err := c.handleModifyRequest(m); err != nil {

client/client_test.go

@@ -1106,3 +1106,33 @@ func TestGet(t *testing.T) {
 		})
 	}
 }
+
+func TestOpResultString(t *testing.T) {
+	tests := []struct {
+		desc     string
+		inResult *OpResult
+		want     string
+	}{{
+		desc:     "nil input",
+		inResult: nil,
+		want:     "<nil>",
+	}, {
+		desc:     "all fields nil",
+		inResult: &OpResult{},
+		want:     "<0 (0 nsec):>",
+	}, {
+		desc: "nil type in details",
+		inResult: &OpResult{
+			OperationID: 42,
+		},
+		want: "<0 (0 nsec): AFTOperation { ID: 42, Type: Unknown, Status: UNSET }>",
+	}}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			if got := tt.inResult.String(); got != tt.want {
+				t.Fatalf("did not get expected string, got: %s, want: %s", got, tt.want)
+			}
+		})
+	}
+}

cmd/ccli/ccli_test.go

@@ -34,11 +34,14 @@ import (
 )
 
 var (
-	addr       = flag.String("addr", "", "address of the gRIBI server in the format hostname:port")
-	insecure   = flag.Bool("insecure", false, "dial insecure gRPC (no TLS)")
-	skipverify = flag.Bool("skipverify", true, "allow self-signed TLS certificate; not needed for -insecure")
-	username   = flag.String("username", os.Getenv("USER"), "username to be sent as gRPC metadata")
-	password   = flag.String("password", "", "password to be sent as gRPC metadata")
+	addr              = flag.String("addr", "", "address of the gRIBI server in the format hostname:port")
+	insecure          = flag.Bool("insecure", false, "dial insecure gRPC (no TLS)")
+	skipVerify        = flag.Bool("skip_verify", true, "allow self-signed TLS certificate; not needed for -insecure")
+	username          = flag.String("username", os.Getenv("USER"), "username to be sent as gRPC metadata")
+	password          = flag.String("password", "", "password to be sent as gRPC metadata")
+	initialElectionID = flag.Uint("initial_electionid", 0, "initial election ID to be used")
+	skipFIBACK        = flag.Bool("skip_fiback", false, "skip tests that rely on FIB ACK")
+	skipSrvReorder    = flag.Bool("skip_reordering", false, "skip tests that rely on server side transaction reordering")
 )
 
 // flagCred implements credentials.PerRPCCredentials by populating the
@@ -64,12 +67,16 @@ func TestCompliance(t *testing.T) {
 		return // Test is part of CI, so do not fail here.
 	}
 
+	if *initialElectionID != 0 {
+		compliance.SetElectionID(uint64(*initialElectionID))
+	}
+
 	dialOpts := []grpc.DialOption{grpc.WithBlock()}
 	if *insecure {
 		dialOpts = append(dialOpts, grpc.WithInsecure())
-	} else if *skipverify {
+	} else if *skipVerify {
 		tlsc := credentials.NewTLS(&tls.Config{
-			InsecureSkipVerify: *skipverify,
+			InsecureSkipVerify: *skipVerify,
 		})
 		dialOpts = append(dialOpts, grpc.WithTransportCredentials(tlsc))
 	}
@@ -87,6 +94,14 @@ func TestCompliance(t *testing.T) {
 	stub := spb.NewGRIBIClient(conn)
 
 	for _, tt := range compliance.TestSuite {
+		if skip := *skipFIBACK; skip && tt.In.RequiresFIBACK {
+			continue
+		}
+
+		if skip := *skipSrvReorder; skip && tt.In.RequiresServerReordering {
+			continue
+		}
+
 		t.Run(tt.In.ShortName, func(t *testing.T) {
 			c := fluent.NewClient()
 			c.Connection().WithStub(stub)