From 88432235436d40d20aeb13e3c668cc31fc5f7ebb Mon Sep 17 00:00:00 2001
From: Matthew Sykes <matthew.sykes@gmail.com>
Date: Wed, 8 Jul 2020 18:04:56 -0400
Subject: [PATCH] Back-fill tests for externalbuilder.Duration

- Use type instead of boxing time.Duration
- Restructure round-trip marshalling tests to table
- Backfill tests to verify behavior of numeric durations decoding

Signed-off-by: Matthew Sykes <matthew.sykes@gmail.com>
---
 core/container/externalbuilder/instance.go    | 22 +++++-----
 .../externalbuilder/instance_test.go          | 42 +++++++++++++++++--
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/core/container/externalbuilder/instance.go b/core/container/externalbuilder/instance.go
index 6547d071c54..f3239fe35ed 100644
--- a/core/container/externalbuilder/instance.go
+++ b/core/container/externalbuilder/instance.go
@@ -35,12 +35,10 @@ type Instance struct {
 }
 
 // Duration used for the DialTimeout property
-type Duration struct {
-	time.Duration
-}
+type Duration time.Duration
 
 func (d Duration) MarshalJSON() ([]byte, error) {
-	return json.Marshal(d.Seconds())
+	return json.Marshal(time.Duration(d).String())
 }
 
 func (d *Duration) UnmarshalJSON(b []byte) error {
@@ -51,17 +49,18 @@ func (d *Duration) UnmarshalJSON(b []byte) error {
 
 	switch value := v.(type) {
 	case float64:
-		d.Duration = time.Duration(value)
-		return nil
+		*d = Duration(time.Duration(value))
 	case string:
-		var err error
-		if d.Duration, err = time.ParseDuration(value); err != nil {
+		dur, err := time.ParseDuration(value)
+		if err != nil {
 			return err
 		}
-		return nil
+		*d = Duration(dur)
 	default:
 		return errors.New("invalid duration")
 	}
+
+	return nil
 }
 
 // ChaincodeServerUserData holds "connection.json" information
@@ -82,10 +81,9 @@ func (c *ChaincodeServerUserData) ChaincodeServerInfo(cryptoDir string) (*ccintf
 	}
 	connInfo := &ccintf.ChaincodeServerInfo{Address: c.Address}
 
-	if c.DialTimeout == (Duration{}) {
+	connInfo.ClientConfig.Timeout = time.Duration(c.DialTimeout)
+	if connInfo.ClientConfig.Timeout == 0 {
 		connInfo.ClientConfig.Timeout = DialTimeout
-	} else {
-		connInfo.ClientConfig.Timeout = c.DialTimeout.Duration
 	}
 
 	// we can expose this if necessary
diff --git a/core/container/externalbuilder/instance_test.go b/core/container/externalbuilder/instance_test.go
index d0809c4ac9d..c2766e29716 100644
--- a/core/container/externalbuilder/instance_test.go
+++ b/core/container/externalbuilder/instance_test.go
@@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
 package externalbuilder_test
 
 import (
+	"encoding/json"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -14,14 +15,16 @@ import (
 	"time"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
-	"go.uber.org/zap"
-	"go.uber.org/zap/zapcore"
+	"github.com/onsi/gomega/types"
 
 	"github.com/hyperledger/fabric/common/flogging"
 	"github.com/hyperledger/fabric/core/container/ccintf"
 	"github.com/hyperledger/fabric/core/container/externalbuilder"
 	"github.com/hyperledger/fabric/internal/pkg/comm"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
 )
 
 var _ = Describe("Instance", func() {
@@ -125,7 +128,7 @@ var _ = Describe("Instance", func() {
 
 			ccuserdata = &externalbuilder.ChaincodeServerUserData{
 				Address:            "ccaddress:12345",
-				DialTimeout:        externalbuilder.Duration{10 * time.Second},
+				DialTimeout:        externalbuilder.Duration(10 * time.Second),
 				TLSRequired:        true,
 				ClientAuthRequired: true,
 				ClientKey:          "fake-key",
@@ -177,7 +180,7 @@ var _ = Describe("Instance", func() {
 
 			Context("dial timeout not provided", func() {
 				It("returns default dial timeout without dialtimeout", func() {
-					ccuserdata.DialTimeout = externalbuilder.Duration{}
+					ccuserdata.DialTimeout = 0
 
 					ccinfo, err := ccuserdata.ChaincodeServerInfo(releaseDir)
 					Expect(err).NotTo(HaveOccurred())
@@ -236,6 +239,37 @@ var _ = Describe("Instance", func() {
 		})
 	})
 
+	Describe("Duration", func() {
+		DescribeTable("Unmarshal",
+			func(input string, expected externalbuilder.Duration, errMatcher types.GomegaMatcher) {
+				var d externalbuilder.Duration
+				err := json.Unmarshal([]byte(input), &d)
+				Expect(err).To(errMatcher)
+			},
+			Entry("Number", `100`, externalbuilder.Duration(100), BeNil()),
+			Entry("Duration", `"1s"`, externalbuilder.Duration(time.Second), BeNil()),
+			Entry("List", `[1, 2, 3]`, externalbuilder.Duration(time.Second), MatchError("invalid duration")),
+			Entry("Nonsense", `"nonsense"`, externalbuilder.Duration(time.Second), MatchError(MatchRegexp(`time: invalid duration "?nonsense"?`))),
+		)
+
+		DescribeTable("Round Trip",
+			func(d time.Duration) {
+				marshalled, err := json.Marshal(externalbuilder.Duration(d))
+				Expect(err).NotTo(HaveOccurred())
+
+				var unmarshalled externalbuilder.Duration
+				err = json.Unmarshal(marshalled, &unmarshalled)
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(unmarshalled).To(Equal(externalbuilder.Duration(d)))
+			},
+			Entry("10ms", 10*time.Millisecond),
+			Entry("10s", 10*time.Second),
+			Entry("10m", 10*time.Minute),
+			Entry("10h", 10*time.Hour),
+		)
+	})
+
 	Describe("Start", func() {
 		It("invokes the builder's run command and sets the run status", func() {
 			err := instance.Start(&ccintf.PeerConnection{