Fix file transfer tests and comments

fileTransfer/connect/listener.go

@@ -21,8 +21,8 @@ const (
 // Name of listener (used for debugging)
 const listenerName = "NewFileTransferListener-Connection"
 
-// listener waits for a message indicating a new file transfer is starting.
-// Adheres to the receive.Listener interface.
+// listener waits for a message indicating a new file transfer is starting. This
+// structure adheres to the [receive.Listener] interface.
 type listener struct {
 	m *Wrapper
 }

fileTransfer/connect/params.go

@@ -19,11 +19,7 @@ const (
 type Params struct {
 	// NotifyUponCompletion indicates if a final notification message is sent
 	// to the recipient on completion of file transfer. If true, the ping is
-	NotifyUponCompletion bool
-}
-
-// paramsDisk will be the marshal-able and unmarshalable object.
-type paramsDisk struct {
+	// sent.
 	NotifyUponCompletion bool
 }
 
@@ -34,8 +30,10 @@ func DefaultParams() Params {
 	}
 }
 
-// GetParameters returns the default Params, or override with given parameters,
-// if set.
+// GetParameters returns the default network parameters, or override with given
+// parameters, if set. Returns an error if provided invalid JSON. If the JSON is
+// valid but does not match the Params structure, the default parameters will be
+// returned.
 func GetParameters(params string) (Params, error) {
 	p := DefaultParams()
 	if len(params) > 0 {
@@ -46,23 +44,3 @@ func GetParameters(params string) (Params, error) {
 	}
 	return p, nil
 }
-
-// MarshalJSON adheres to the json.Marshaler interface.
-func (p Params) MarshalJSON() ([]byte, error) {
-	pDisk := paramsDisk{NotifyUponCompletion: p.NotifyUponCompletion}
-	return json.Marshal(&pDisk)
-
-}
-
-// UnmarshalJSON adheres to the json.Unmarshaler interface.
-func (p *Params) UnmarshalJSON(data []byte) error {
-	pDisk := paramsDisk{}
-	err := json.Unmarshal(data, &pDisk)
-	if err != nil {
-		return err
-	}
-
-	*p = Params{NotifyUponCompletion: pDisk.NotifyUponCompletion}
-
-	return nil
-}

fileTransfer/connect/params_test.go

+////////////////////////////////////////////////////////////////////////////////
+// Copyright © 2022 xx foundation                                             //
+//                                                                            //
+// Use of this source code is governed by a license that can be found in the  //
+// LICENSE file.                                                              //
+////////////////////////////////////////////////////////////////////////////////
+
+package connect
+
+import (
+	"encoding/json"
+	"reflect"
+	"testing"
+)
+
+// Tests that DefaultParams returns a Params object with the expected defaults.
+func TestDefaultParams(t *testing.T) {
+	expected := Params{
+		NotifyUponCompletion: defaultNotifyUponCompletion,
+	}
+
+	received := DefaultParams()
+	if !reflect.DeepEqual(expected, received) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %+v\nreceived: %+v", expected, received)
+	}
+}
+
+// Tests that GetParameters uses the passed in parameters.
+func TestGetParameters(t *testing.T) {
+	expected := Params{
+		NotifyUponCompletion: false,
+	}
+	expectedData, err := json.Marshal(expected)
+	if err != nil {
+		t.Errorf("Failed to JSON marshal expected params: %+v", err)
+	}
+
+	p, err := GetParameters(string(expectedData))
+	if err != nil {
+		t.Errorf("Failed get parameters: %+v", err)
+	}
+
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
+}
+
+// Tests that GetParameters returns the default parameters if no params string
+// is provided
+func TestGetParameters_Default(t *testing.T) {
+	expected := DefaultParams()
+
+	p, err := GetParameters("")
+	if err != nil {
+		t.Errorf("Failed get parameters: %+v", err)
+	}
+
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
+}
+
+// Error path: Tests that GetParameters returns an error when the params string
+// does not contain a valid JSON representation of Params.
+func TestGetParameters_InvalidParamsStringError(t *testing.T) {
+	_, err := GetParameters("invalid JSON")
+	if err == nil {
+		t.Error("Failed get get error for invalid JSON")
+	}
+}
+
+// Tests that a Params object marshalled via json.Marshal and unmarshalled via
+// json.Unmarshal matches the original.
+func TestParams_JsonMarshalUnmarshal(t *testing.T) {
+	// Construct a set of params
+	expected := DefaultParams()
+
+	// Marshal the params
+	data, err := json.Marshal(&expected)
+	if err != nil {
+		t.Fatalf("Marshal error: %v", err)
+	}
+
+	// Unmarshal the params object
+	received := Params{}
+	err = json.Unmarshal(data, &received)
+	if err != nil {
+		t.Fatalf("Unmarshal error: %v", err)
+	}
+
+	if !reflect.DeepEqual(expected, received) {
+		t.Errorf("Marshalled and unmarshalled Params does not match original."+
+			"\nexpected: %#v\nreceived: %#v", expected, received)
+	}
+}

fileTransfer/connect/wrapper.go

@@ -35,8 +35,8 @@ type Wrapper struct {
 	conn connection
 }
 
-// connection interface matches a subset of the connect.Connection methods used
-// by the Wrapper for easier testing.
+// connection interface matches a subset of the [connect.Connection] methods
+// used by the Wrapper for easier testing.
 type connection interface {
 	GetPartner() partner.Manager
 	SendE2E(mt catalog.MessageType, payload []byte, params e2e.Params) (
@@ -118,7 +118,8 @@ func (w *Wrapper) RegisterSentProgressCallback(tid *ftCrypto.TransferID,
 // addEndMessageToCallback adds the sending of a connection E2E message when
 // the transfer completed to the callback. If NotifyUponCompletion is not set,
 // then the message is not sent.
-func (w *Wrapper) addEndMessageToCallback(progressCB ft.SentProgressCallback) ft.SentProgressCallback {
+func (w *Wrapper) addEndMessageToCallback(
+	progressCB ft.SentProgressCallback) ft.SentProgressCallback {
 	if !w.p.NotifyUponCompletion {
 		return progressCB
 	}

fileTransfer/connect/wrapper_test.go

@@ -25,7 +25,7 @@ import (
 	"time"
 )
 
-// Tests that Connection adheres to the connect.Connection interface.
+// Tests that Connection adheres to the [connect.Connection] interface.
 var _ connection = (connect.Connection)(nil)
 
 // Smoke test of the entire file transfer system.
@@ -60,7 +60,8 @@ func Test_FileTransfer_Smoke(t *testing.T) {
 	storage1 := newMockStorage()
 	endE2eChan1 := make(chan receive.Message, 3)
 	conn1 := newMockConnection(myID1, myID2, e2eHandler, t)
-	_, _ = conn1.RegisterListener(catalog.EndFileTransfer, newMockListener(endE2eChan1))
+	_, _ = conn1.RegisterListener(
+		catalog.EndFileTransfer, newMockListener(endE2eChan1))
 	cMix1 := newMockCmix(myID1, cMixHandler, storage1)
 	user1 := newMockUser(myID1, cMix1, storage1, rngGen)
 	ftManager1, err := ft.NewManager(ftParams, user1)
@@ -86,7 +87,8 @@ func Test_FileTransfer_Smoke(t *testing.T) {
 	storage2 := newMockStorage()
 	endE2eChan2 := make(chan receive.Message, 3)
 	conn2 := newMockConnection(myID2, myID1, e2eHandler, t)
-	_, _ = conn2.RegisterListener(catalog.EndFileTransfer, newMockListener(endE2eChan2))
+	_, _ = conn2.RegisterListener(
+		catalog.EndFileTransfer, newMockListener(endE2eChan2))
 	cMix2 := newMockCmix(myID1, cMixHandler, storage2)
 	user2 := newMockUser(myID2, cMix2, storage2, rngGen)
 	ftManager2, err := ft.NewManager(ftParams, user2)

fileTransfer/e2e/listener.go

@@ -21,8 +21,8 @@ const (
 // Name of listener (used for debugging)
 const listenerName = "NewFileTransferListener-E2E"
 
-// listener waits for a message indicating a new file transfer is starting.
-// Adheres to the receive.Listener interface.
+// listener waits for a message indicating a new file transfer is starting. This
+// structure adheres to the [receive.Listener] interface.
 type listener struct {
 	m *Wrapper
 }

fileTransfer/e2e/params.go

@@ -17,11 +17,7 @@ const (
 type Params struct {
 	// NotifyUponCompletion indicates if a final notification message is sent
 	// to the recipient on completion of file transfer. If true, the ping is
-	NotifyUponCompletion bool
-}
-
-// paramsDisk will be the marshal-able and umarshal-able object.
-type paramsDisk struct {
+	// sent.
 	NotifyUponCompletion bool
 }
 
@@ -32,8 +28,10 @@ func DefaultParams() Params {
 	}
 }
 
-// GetParameters returns the default Params, or override with given
-// parameters, if set.
+// GetParameters returns the default network parameters, or override with given
+// parameters, if set. Returns an error if provided invalid JSON. If the JSON is
+// valid but does not match the Params structure, the default parameters will be
+// returned.
 func GetParameters(params string) (Params, error) {
 	p := DefaultParams()
 	if len(params) > 0 {
@@ -44,23 +42,3 @@ func GetParameters(params string) (Params, error) {
 	}
 	return p, nil
 }
-
-// MarshalJSON adheres to the json.Marshaler interface.
-func (p Params) MarshalJSON() ([]byte, error) {
-	pDisk := paramsDisk{NotifyUponCompletion: p.NotifyUponCompletion}
-	return json.Marshal(&pDisk)
-
-}
-
-// UnmarshalJSON adheres to the json.Unmarshaler interface.
-func (p *Params) UnmarshalJSON(data []byte) error {
-	pDisk := paramsDisk{}
-	err := json.Unmarshal(data, &pDisk)
-	if err != nil {
-		return err
-	}
-
-	*p = Params{NotifyUponCompletion: pDisk.NotifyUponCompletion}
-
-	return nil
-}

fileTransfer/e2e/params_test.go

+////////////////////////////////////////////////////////////////////////////////
+// Copyright © 2022 xx foundation                                             //
+//                                                                            //
+// Use of this source code is governed by a license that can be found in the  //
+// LICENSE file.                                                              //
+////////////////////////////////////////////////////////////////////////////////
+
+package e2e
+
+import (
+	"encoding/json"
+	"reflect"
+	"testing"
+)
+
+// Tests that DefaultParams returns a Params object with the expected defaults.
+func TestDefaultParams(t *testing.T) {
+	expected := Params{
+		NotifyUponCompletion: defaultNotifyUponCompletion,
+	}
+
+	received := DefaultParams()
+	if !reflect.DeepEqual(expected, received) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %+v\nreceived: %+v", expected, received)
+	}
+}
+
+// Tests that GetParameters uses the passed in parameters.
+func TestGetParameters(t *testing.T) {
+	expected := Params{
+		NotifyUponCompletion: false,
+	}
+	expectedData, err := json.Marshal(expected)
+	if err != nil {
+		t.Errorf("Failed to JSON marshal expected params: %+v", err)
+	}
+
+	p, err := GetParameters(string(expectedData))
+	if err != nil {
+		t.Errorf("Failed get parameters: %+v", err)
+	}
+
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
+}
+
+// Tests that GetParameters returns the default parameters if no params string
+// is provided
+func TestGetParameters_Default(t *testing.T) {
+	expected := DefaultParams()
+
+	p, err := GetParameters("")
+	if err != nil {
+		t.Errorf("Failed get parameters: %+v", err)
+	}
+
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
+}
+
+// Error path: Tests that GetParameters returns an error when the params string
+// does not contain a valid JSON representation of Params.
+func TestGetParameters_InvalidParamsStringError(t *testing.T) {
+	_, err := GetParameters("invalid JSON")
+	if err == nil {
+		t.Error("Failed get get error for invalid JSON")
+	}
+}
+
+// Tests that a Params object marshalled via json.Marshal and unmarshalled via
+// json.Unmarshal matches the original.
+func TestParams_JsonMarshalUnmarshal(t *testing.T) {
+	// Construct a set of params
+	expected := DefaultParams()
+
+	// Marshal the params
+	data, err := json.Marshal(&expected)
+	if err != nil {
+		t.Fatalf("Marshal error: %v", err)
+	}
+
+	// Unmarshal the params object
+	received := Params{}
+	err = json.Unmarshal(data, &received)
+	if err != nil {
+		t.Fatalf("Unmarshal error: %v", err)
+	}
+
+	if !reflect.DeepEqual(expected, received) {
+		t.Errorf("Marshalled and unmarshalled Params does not match original."+
+			"\nexpected: %#v\nreceived: %#v", expected, received)
+	}
+}

fileTransfer/e2e/send.go

@@ -57,7 +57,8 @@ func sendNewFileTransferMessage(
 
 // sendEndFileTransferMessage sends an E2E message to the recipient informing
 // them that all file parts have arrived once the network is healthy.
-func sendEndFileTransferMessage(recipient *id.ID, cmix ft.Cmix, e2eHandler e2eHandler) {
+func sendEndFileTransferMessage(
+	recipient *id.ID, cmix ft.Cmix, e2eHandler e2eHandler) {
 	callbackID := make(chan uint64, 1)
 	callbackID <- cmix.AddHealthCallback(
 		func(healthy bool) {

fileTransfer/e2e/wrapper.go

@@ -35,8 +35,8 @@ type Wrapper struct {
 	e2e  e2eHandler
 }
 
-// e2eHandler interface matches a subset of the e2e.Handler methods used by the Wrapper
-// for easier testing.
+// e2eHandler interface matches a subset of the [e2e.Handler] methods used by
+// the Wrapper for easier testing.
 type e2eHandler interface {
 	SendE2E(mt catalog.MessageType, recipient *id.ID, payload []byte,
 		params e2e.Params) (cryptoE2e.SendReport, error)

fileTransfer/groupChat/wrapper.go

@@ -49,8 +49,8 @@ type gcManager interface {
 }
 
 // NewWrapper generates a new file transfer Wrapper for group chat.
-func NewWrapper(receiveCB ft.ReceiveCallback, ft ft.FileTransfer, gc gcManager) (
-	*Wrapper, error) {
+func NewWrapper(receiveCB ft.ReceiveCallback, ft ft.FileTransfer,
+	gc gcManager) (*Wrapper, error) {
 	w := &Wrapper{
 		receiveCB: receiveCB,
 		ft:        ft,

fileTransfer/manager_test.go

@@ -204,9 +204,11 @@ func Test_FileTransfer_Smoke(t *testing.T) {
 				fileName, sendTime, fileSizeKb, throughput)
 
 			expectedThroughput := float64(params.MaxThroughput) * .001
-			delta := (math.Abs(expectedThroughput-throughput) / ((expectedThroughput + throughput) / 2)) * 100
+			delta := (math.Abs(expectedThroughput-throughput) /
+				((expectedThroughput + throughput) / 2)) * 100
 			t.Logf("Expected bandwidth:   %.2f kb/s", expectedThroughput)
-			t.Logf("Bandwidth difference: %.2f kb/s (%.2f%%)", expectedThroughput-throughput, delta)
+			t.Logf("Bandwidth difference: %.2f kb/s (%.2f%%)",
+				expectedThroughput-throughput, delta)
 		}
 	}()
 

fileTransfer/params.go

@@ -33,13 +33,6 @@ type Params struct {
 	Cmix cmix.CMIXParams
 }
 
-// paramsDisk will be the marshal-able and umarshal-able object.
-type paramsDisk struct {
-	MaxThroughput int
-	SendTimeout   time.Duration
-	Cmix          cmix.CMIXParams
-}
-
 // DefaultParams returns a Params object filled with the default values.
 func DefaultParams() Params {
 	return Params{
@@ -50,7 +43,9 @@ func DefaultParams() Params {
 }
 
 // GetParameters returns the default network parameters, or override with given
-// parameters, if set.
+// parameters, if set. Returns an error if provided invalid JSON. If the JSON is
+// valid but does not match the Params structure, the default parameters will be
+// returned.
 func GetParameters(params string) (Params, error) {
 	p := DefaultParams()
 	if len(params) > 0 {
@@ -61,32 +56,3 @@ func GetParameters(params string) (Params, error) {
 	}
 	return p, nil
 }
-
-// MarshalJSON adheres to the json.Marshaler interface.
-func (p Params) MarshalJSON() ([]byte, error) {
-	pDisk := paramsDisk{
-		MaxThroughput: p.MaxThroughput,
-		SendTimeout:   p.SendTimeout,
-		Cmix:          p.Cmix,
-	}
-
-	return json.Marshal(&pDisk)
-
-}
-
-// UnmarshalJSON adheres to the json.Unmarshaler interface.
-func (p *Params) UnmarshalJSON(data []byte) error {
-	pDisk := paramsDisk{}
-	err := json.Unmarshal(data, &pDisk)
-	if err != nil {
-		return err
-	}
-
-	*p = Params{
-		MaxThroughput: pDisk.MaxThroughput,
-		SendTimeout:   pDisk.SendTimeout,
-		Cmix:          pDisk.Cmix,
-	}
-
-	return nil
-}

fileTransfer/params_test.go

@@ -8,58 +8,111 @@
 package fileTransfer
 
 import (
-	"bytes"
 	"encoding/json"
 	"gitlab.com/elixxir/client/cmix"
 	"reflect"
 	"testing"
 )
 
-// Tests that no data is lost when marshaling and unmarshalling the Params
-// object.
-func TestParams_MarshalUnmarshal(t *testing.T) {
-	// Construct a set of params
-	p := DefaultParams()
+// Tests that DefaultParams returns a Params object with the expected defaults.
+func TestDefaultParams(t *testing.T) {
+	expected := Params{
+		MaxThroughput: defaultMaxThroughput,
+		SendTimeout:   defaultSendTimeout,
+		Cmix:          cmix.GetDefaultCMIXParams(),
+	}
+	received := DefaultParams()
+	received.Cmix.Stop = expected.Cmix.Stop
 
-	// Marshal the params
-	data, err := json.Marshal(&p)
+	if !reflect.DeepEqual(expected, received) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %+v\nreceived: %+v", expected, received)
+	}
+}
+
+// Tests that GetParameters uses the passed in parameters.
+func TestGetParameters(t *testing.T) {
+	expected := Params{
+		MaxThroughput: 42,
+		SendTimeout:   11,
+		Cmix: cmix.CMIXParams{
+			RoundTries:       5,
+			Timeout:          6,
+			RetryDelay:       7,
+			ExcludedRounds:   nil,
+			SendTimeout:      8,
+			DebugTag:         "9",
+			Stop:             nil,
+			BlacklistedNodes: cmix.NodeMap{},
+			Critical:         true,
+		},
+	}
+	expectedData, err := json.Marshal(expected)
 	if err != nil {
-		t.Fatalf("Marshal error: %v", err)
+		t.Errorf("Failed to JSON marshal expected params: %+v", err)
 	}
 
-	// Unmarshal the params object
-	received := Params{}
-	err = json.Unmarshal(data, &received)
+	p, err := GetParameters(string(expectedData))
 	if err != nil {
-		t.Fatalf("Unmarshal error: %v", err)
+		t.Errorf("Failed get parameters: %+v", err)
+	}
+	p.Cmix.Stop = expected.Cmix.Stop
+
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
 }
 
-	// Re-marshal this params object
-	data2, err := json.Marshal(received)
+// Tests that GetParameters returns the default parameters if no params string
+// is provided
+func TestGetParameters_Default(t *testing.T) {
+	expected := DefaultParams()
+
+	p, err := GetParameters("")
 	if err != nil {
-		t.Fatalf("Marshal error: %v", err)
+		t.Errorf("Failed get parameters: %+v", err)
 	}
+	p.Cmix.Stop = expected.Cmix.Stop
 
-	// Check that they match (it is done this way to avoid false failures with
-	// the reflect.DeepEqual function and pointers)
-	if !bytes.Equal(data, data2) {
-		t.Fatalf("Data was lost in marshal/unmarshal.")
+	if !reflect.DeepEqual(expected, p) {
+		t.Errorf("Received Params does not match expected."+
+			"\nexpected: %#v\nreceived: %#v", expected, p)
+	}
 }
 
+// Error path: Tests that GetParameters returns an error when the params string
+// does not contain a valid JSON representation of Params.
+func TestGetParameters_InvalidParamsStringError(t *testing.T) {
+	_, err := GetParameters("invalid JSON")
+	if err == nil {
+		t.Error("Failed get get error for invalid JSON")
+	}
 }
 
-// Tests that DefaultParams returns a Params object with the expected defaults.
-func TestDefaultParams(t *testing.T) {
-	expected := Params{
-		MaxThroughput: defaultMaxThroughput,
-		SendTimeout:   defaultSendTimeout,
-		Cmix:          cmix.GetDefaultCMIXParams(),
+// Tests that a Params object marshalled via json.Marshal and unmarshalled via
+// json.Unmarshal matches the original.
+func TestParams_JsonMarshalUnmarshal(t *testing.T) {
+	// Construct a set of params
+	expected := DefaultParams()
+	expected.Cmix.BlacklistedNodes = cmix.NodeMap{}
+
+	// Marshal the params
+	data, err := json.Marshal(&expected)
+	if err != nil {
+		t.Fatalf("Marshal error: %v", err)
+	}
+
+	// Unmarshal the params object
+	received := Params{}
+	err = json.Unmarshal(data, &received)
+	if err != nil {
+		t.Fatalf("Unmarshal error: %v", err)
 	}
-	received := DefaultParams()
-	received.Cmix.Stop = expected.Cmix.Stop
 
+	received.Cmix.Stop = expected.Cmix.Stop
 	if !reflect.DeepEqual(expected, received) {
-		t.Errorf("Received Params does not match expected."+
-			"\nexpected: %+v\nreceived: %+v", expected, received)
+		t.Errorf("Marshalled and unmarshalled Params does not match original."+
+			"\nexpected: %#v\nreceived: %#v", expected, received)
 	}
 }