Tests for change.go now include error channel

build/ci/.test.yml

@@ -27,6 +27,7 @@ integration-test:
   allow_failure: true
   variables:
     GOSDN_LOG: "nolog"
+    GOSDN_CHANGE_TIMEOUT: "100ms"
   rules:
     - if: $CI_PIPELINE_SOURCE == "merge_request_event" && $CI_MERGE_REQUEST_TARGET_BRANCH_NAME == $CI_DEFAULT_BRANCH
     - if: $CI_NIGHTLY

mocks/PrincipalNetworkDomain.go

@@ -64,27 +64,48 @@ func (_m *PrincipalNetworkDomain) ChangeOND(_a0 uuid.UUID, operation interface{}
 	return r0
 }
 
-// Committed provides a mock function with given fields: _a0
-func (_m *PrincipalNetworkDomain) Committed(_a0 uuid.UUID) (interface{}, error) {
+// Commit provides a mock function with given fields: _a0
+func (_m *PrincipalNetworkDomain) Commit(_a0 uuid.UUID) error {
 	ret := _m.Called(_a0)
 
-	var r0 interface{}
-	if rf, ok := ret.Get(0).(func(uuid.UUID) interface{}); ok {
+	var r0 error
+	if rf, ok := ret.Get(0).(func(uuid.UUID) error); ok {
 		r0 = rf(_a0)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// Committed provides a mock function with given fields:
+func (_m *PrincipalNetworkDomain) Committed() []uuid.UUID {
+	ret := _m.Called()
+
+	var r0 []uuid.UUID
+	if rf, ok := ret.Get(0).(func() []uuid.UUID); ok {
+		r0 = rf()
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(interface{})
+			r0 = ret.Get(0).([]uuid.UUID)
 		}
 	}
 
-	var r1 error
-	if rf, ok := ret.Get(1).(func(uuid.UUID) error); ok {
-		r1 = rf(_a0)
+	return r0
+}
+
+// Confirm provides a mock function with given fields: _a0
+func (_m *PrincipalNetworkDomain) Confirm(_a0 uuid.UUID) error {
+	ret := _m.Called(_a0)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(uuid.UUID) error); ok {
+		r0 = rf(_a0)
 	} else {
-		r1 = ret.Error(1)
+		r0 = ret.Error(0)
 	}
 
-	return r0, r1
+	return r0
 }
 
 // ContainsDevice provides a mock function with given fields: _a0
@@ -115,6 +136,22 @@ func (_m *PrincipalNetworkDomain) Destroy() error {
 	return r0
 }
 
+// Devices provides a mock function with given fields:
+func (_m *PrincipalNetworkDomain) Devices() []uuid.UUID {
+	ret := _m.Called()
+
+	var r0 []uuid.UUID
+	if rf, ok := ret.Get(0).(func() []uuid.UUID); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]uuid.UUID)
+		}
+	}
+
+	return r0
+}
+
 // GetDescription provides a mock function with given fields:
 func (_m *PrincipalNetworkDomain) GetDescription() string {
 	ret := _m.Called()
@@ -198,38 +235,6 @@ func (_m *PrincipalNetworkDomain) ID() uuid.UUID {
 	return r0
 }
 
-// ListCommitted provides a mock function with given fields:
-func (_m *PrincipalNetworkDomain) ListCommitted() []uuid.UUID {
-	ret := _m.Called()
-
-	var r0 []uuid.UUID
-	if rf, ok := ret.Get(0).(func() []uuid.UUID); ok {
-		r0 = rf()
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).([]uuid.UUID)
-		}
-	}
-
-	return r0
-}
-
-// ListPending provides a mock function with given fields:
-func (_m *PrincipalNetworkDomain) ListPending() []uuid.UUID {
-	ret := _m.Called()
-
-	var r0 []uuid.UUID
-	if rf, ok := ret.Get(0).(func() []uuid.UUID); ok {
-		r0 = rf()
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).([]uuid.UUID)
-		}
-	}
-
-	return r0
-}
-
 // MarshalDevice provides a mock function with given fields: _a0
 func (_m *PrincipalNetworkDomain) MarshalDevice(_a0 uuid.UUID) (string, error) {
 	ret := _m.Called(_a0)
@@ -251,27 +256,20 @@ func (_m *PrincipalNetworkDomain) MarshalDevice(_a0 uuid.UUID) (string, error) {
 	return r0, r1
 }
 
-// Pending provides a mock function with given fields: _a0
-func (_m *PrincipalNetworkDomain) Pending(_a0 uuid.UUID) (interface{}, error) {
-	ret := _m.Called(_a0)
+// Pending provides a mock function with given fields:
+func (_m *PrincipalNetworkDomain) Pending() []uuid.UUID {
+	ret := _m.Called()
 
-	var r0 interface{}
-	if rf, ok := ret.Get(0).(func(uuid.UUID) interface{}); ok {
-		r0 = rf(_a0)
+	var r0 []uuid.UUID
+	if rf, ok := ret.Get(0).(func() []uuid.UUID); ok {
+		r0 = rf()
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(interface{})
+			r0 = ret.Get(0).([]uuid.UUID)
 		}
 	}
 
-	var r1 error
-	if rf, ok := ret.Get(1).(func(uuid.UUID) error); ok {
-		r1 = rf(_a0)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
+	return r0
 }
 
 // RemoveDevice provides a mock function with given fields: _a0

nucleus/http.go

@@ -2,7 +2,6 @@ package nucleus
 
 import (
 	"code.fbi.h-da.de/cocsn/gosdn/forks/goarista/gnmi"
-	. "code.fbi.h-da.de/cocsn/gosdn/nucleus/pnd"
 	"context"
 	"fmt"
 	"github.com/google/uuid"
@@ -179,7 +178,7 @@ func httpHandler(writer http.ResponseWriter, request *http.Request) {
 				handleServerError(writer, err)
 				return
 			}
-			writeIDs(writer, "Devices", p.(*pndImplementation).devices.UUIDs())
+			writeIDs(writer, "Devices", p.Devices())
 		}
 	case "init":
 		writeIDs(writer, "PNDs", c.pndc.UUIDs())
@@ -197,10 +196,10 @@ func httpHandler(writer http.ResponseWriter, request *http.Request) {
 		}
 		writer.WriteHeader(http.StatusOK)
 	case "change-list":
-		changes := pnd.ListCommitted()
+		changes := pnd.Committed()
 		writeIDs(writer, "Tentative changes", changes)
 	case "change-list-pending":
-		changes := pnd.ListPending()
+		changes := pnd.Pending()
 		writeIDs(writer, "Pending changes", changes)
 	case "change-commit":
 		cuid, err := uuid.Parse(query.Get("cuid"))
@@ -208,13 +207,7 @@ func httpHandler(writer http.ResponseWriter, request *http.Request) {
 			handleServerError(writer, err)
 			return
 		}
-		change, err := pnd.Pending(cuid)
-		if err != nil {
-			handleServerError(writer, err)
-			return
-		}
-		err = change.(*Change).Commit()
-		if err != nil {
+		if err := pnd.Commit(cuid); err != nil {
 			handleServerError(writer, err)
 			return
 		}
@@ -225,13 +218,7 @@ func httpHandler(writer http.ResponseWriter, request *http.Request) {
 			handleServerError(writer, err)
 			return
 		}
-		change, err := pnd.Committed(cuid)
-		if err != nil {
-			handleServerError(writer, err)
-			return
-		}
-		err = change.(*Change).Confirm()
-		if err != nil {
+		if err := pnd.Confirm(cuid); err != nil {
 			handleServerError(writer, err)
 			return
 		}

nucleus/http_test.go

@@ -150,19 +150,21 @@ func Test_httpApi(t *testing.T) {
 		},
 		{
 			name:    "change commit",
-			request: apiEndpoint + "/api?q=change-commit" + args + "&cuid=" + uuid.New().String(),
-			want:    &http.Response{StatusCode: http.StatusOK},
+			request: apiEndpoint + "/api?q=change-commit" + args + "&cuid=" + cuid.String(),
+			// TODO: Mock Change for testing
+			want:    &http.Response{StatusCode: http.StatusInternalServerError},
 			wantErr: false,
 		},
 		{
 			name:    "change confirm",
-			request: apiEndpoint + "/api?q=change-confirm" + args + "&cuid="  + uuid.New().String(),
-			want:    &http.Response{StatusCode: http.StatusOK},
+			request: apiEndpoint + "/api?q=change-confirm" + args + "&cuid=" + cuid.String(),
+			// TODO: Mock Change for testing
+			want:    &http.Response{StatusCode: http.StatusInternalServerError},
 			wantErr: false,
 		},
 		{
 			name:    "bad request",
-			request: apiEndpoint + "/api?q=bad-request",
+			request: apiEndpoint + "/api?q=bad-request" + args,
 			want:    &http.Response{StatusCode: http.StatusBadRequest},
 			wantErr: false,
 		},

nucleus/initialise_test.go

@@ -25,6 +25,7 @@ var defaultPndID uuid.UUID
 var ocUUID uuid.UUID
 var iid uuid.UUID
 var altIid uuid.UUID
+var cuid uuid.UUID
 
 var sbi SouthboundInterface
 var pnd PrincipalNetworkDomain
@@ -114,6 +115,7 @@ func readTestUUIDs() {
 	ocUUID, err = uuid.Parse("5e252b70-38f2-4c99-a0bf-1b16af4d7e67")
 	iid, err = uuid.Parse("8495a8ac-a1e8-418e-b787-10f5878b2690")
 	altIid, err = uuid.Parse("edc5de93-2d15-4586-b2a7-fb1bc770986b")
+	cuid, err = uuid.Parse("3e8219b0-e926-400d-8660-217f2a25a7c6")
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -139,5 +141,6 @@ func newPnd() pndImplementation {
 		committedChanges: changeStore{store{}},
 		confirmedChanges: changeStore{store{}},
 		id:               defaultPndID,
+		errChans:         make(map[uuid.UUID]chan error),
 	}
 }

nucleus/pnd/change.go

@@ -14,23 +14,23 @@ import (
 var changeTimeout time.Duration
 
 func init() {
-	timeout, err := time.ParseDuration(os.Getenv("GOSDN_CHANGE_TIMEOUT"))
-	if err != nil {
-		log.Fatal(err)
-	}
-	if timeout != time.Duration(0) {
-		changeTimeout = timeout
+	var err error
+	e := os.Getenv("GOSDN_CHANGE_TIMEOUT")
+	if e != "" {
+		changeTimeout, err = time.ParseDuration(e)
+		if err != nil {
+			log.Fatal(err)
+		}
 	} else {
-		var err error
 		changeTimeout, err = time.ParseDuration("10m")
 		if err != nil {
-			log.Fatal()
+			log.Fatal(err)
 		}
 	}
 	log.Debugf("change timeout set to %v", changeTimeout)
 }
 
-func NewChange(device uuid.UUID, currentState ygot.GoStruct, change ygot.GoStruct, callback func(ygot.GoStruct, ygot.GoStruct) error) *Change {
+func NewChange(device uuid.UUID, currentState ygot.GoStruct, change ygot.GoStruct, callback func(ygot.GoStruct, ygot.GoStruct) error, errChan chan error) *Change {
 	return &Change{
 		cuid:          uuid.New(),
 		duid:          device,
@@ -40,6 +40,8 @@ func NewChange(device uuid.UUID, currentState ygot.GoStruct, change ygot.GoStruc
 		committed:     false,
 		confirmed:     false,
 		callback:      callback,
+		errChan:       errChan,
+		Done:          make(chan int),
 	}
 }
 
@@ -55,9 +57,13 @@ type Change struct {
 	intendedState ygot.GoStruct
 	committed     bool
 	confirmed     bool
+	inconsistent  bool
 	callback      func(ygot.GoStruct, ygot.GoStruct) error
 	lock          sync.RWMutex
 	cancelFunc    context.CancelFunc
+	errChan       chan error
+	// TODO: Move nucleus.pndImplementation and Change to same package and unexport
+	Done chan int
 }
 
 func (c *Change) ID() uuid.UUID {
@@ -65,10 +71,10 @@ func (c *Change) ID() uuid.UUID {
 }
 
 func (c *Change) Commit() error {
-	c.committed = true
 	if err := c.callback(c.intendedState, c.previousState); err != nil {
 		return err
 	}
+	c.committed = true
 	log.WithFields(log.Fields{
 		"change uuid": c.cuid,
 		"device uuid": c.duid,
@@ -87,14 +93,7 @@ func (c *Change) rollbackHandler(ctx context.Context) {
 		c.lock.RLock()
 		defer c.lock.RUnlock()
 		if !c.confirmed {
-			err := c.callback(c.previousState, c.intendedState)
-			if err != nil {
-				log.WithFields(log.Fields{
-					"change uuid": c.cuid,
-					"device uuid": c.duid,
-					"error":       err,
-				}).Error("rollback error")
-			}
+			c.errChan <- c.callback(c.previousState, c.intendedState)
 			log.WithFields(log.Fields{
 				"change uuid": c.cuid,
 				"device uuid": c.duid,
@@ -114,6 +113,9 @@ func (c *Change) Confirm() error {
 	defer c.lock.Unlock()
 	c.confirmed = true
 	c.cancelFunc()
+	close(c.errChan)
+	c.Done <- 0
+	close(c.Done)
 	log.WithFields(log.Fields{
 		"change uuid": c.cuid,
 		"device uuid": c.duid,

nucleus/pnd/change_test.go

@@ -2,10 +2,10 @@ package pnd
 
 import (
 	"context"
+	"errors"
 	"github.com/google/uuid"
 	"github.com/openconfig/ygot/exampleoc"
 	"github.com/openconfig/ygot/ygot"
-	"os"
 	"reflect"
 	"sync"
 	"testing"
@@ -53,11 +53,7 @@ func TestChange_CommitRollback(t *testing.T) {
 		if err := c.Commit(); (err != nil) != wantErr {
 			t.Errorf("Commit() error = %v, wantErr %v", err, wantErr)
 		}
-		timeout, err := time.ParseDuration(os.Getenv("GOSDN_CHANGE_TIMEOUT"))
-		if err != nil {
-			t.Error(err)
-		}
-		time.Sleep(timeout)
+		time.Sleep(changeTimeout)
 	}()
 	got := <-callback
 	if !reflect.DeepEqual(got, want) {
@@ -66,6 +62,66 @@ func TestChange_CommitRollback(t *testing.T) {
 	close(callback)
 }
 
+func TestChange_CommitRollbackError(t *testing.T) {
+	wantErr := false
+	want := errors.New("this is an expected error")
+	c := &Change{
+		cuid:          changeUUID,
+		duid:          did,
+		timestamp:     time.Now(),
+		previousState: rollbackDevice,
+		intendedState: commitDevice,
+		callback: func(first ygot.GoStruct, second ygot.GoStruct) error {
+			hostname := *first.(*exampleoc.Device).System.Hostname
+			t.Logf("hostname: %v", hostname)
+			switch hostname {
+			case rollback:
+				return errors.New("this is an expected error")
+			}
+			return nil
+		},
+		lock:    sync.RWMutex{},
+		errChan: make(chan error),
+	}
+	go func() {
+		time.Sleep(time.Millisecond * 10)
+		if err := c.Commit(); (err != nil) != wantErr {
+			t.Errorf("Commit() error = %v, wantErr %v", err, wantErr)
+		}
+		time.Sleep(changeTimeout)
+	}()
+	got := <-c.errChan
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("Commit() = %v, want %v", got, want)
+	}
+	close(c.errChan)
+}
+
+func TestChange_CommitError(t *testing.T) {
+	wantErr := true
+	c := &Change{
+		cuid:          changeUUID,
+		duid:          did,
+		timestamp:     time.Now(),
+		previousState: rollbackDevice,
+		intendedState: commitDevice,
+		callback: func(first ygot.GoStruct, second ygot.GoStruct) error {
+			return errors.New("this is an expected error")
+		},
+		lock: sync.RWMutex{},
+	}
+	go func() {
+		time.Sleep(time.Millisecond * 10)
+		if err := c.Commit(); (err != nil) != wantErr {
+			t.Errorf("Commit() error = %v, wantErr %v", err, wantErr)
+		}
+	}()
+	got := c.committed
+	if !reflect.DeepEqual(got, false) {
+		t.Errorf("Commit() = %v, want %v", got, false)
+	}
+}
+
 func TestChange_Commit(t *testing.T) {
 	wantErr := false
 	want := commit
@@ -83,7 +139,9 @@ func TestChange_Commit(t *testing.T) {
 			callback <- hostname
 			return nil
 		},
-		lock: sync.RWMutex{},
+		lock:    sync.RWMutex{},
+		errChan: make(chan error),
+		Done:    make(chan int),
 	}
 	go func() {
 		time.Sleep(time.Millisecond * 10)
@@ -133,8 +191,7 @@ func TestChange_Confirm(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Change{
-				cuid:      tt.fields.cuid,
-				duid:      tt.fields.duid,
+				committed: tt.fields.committed,
 				timestamp: tt.fields.timestamp,
 				previousState: &exampleoc.Device{
 					System: &exampleoc.System{
@@ -146,10 +203,10 @@ func TestChange_Confirm(t *testing.T) {
 						Hostname: &commit,
 					},
 				},
-				callback:   tt.fields.callback,
-				committed:  tt.fields.committed,
 				cancelFunc: cancel,
 				lock:       sync.RWMutex{},
+				errChan:    make(chan error),
+				Done:       make(chan int, 1),
 			}
 			if err := c.Confirm(); (err != nil) != tt.wantErr {
 				t.Errorf("Confirm() error = %v, wantErr %v", err, tt.wantErr)

nucleus/principalNetworkDomain.go

@@ -20,8 +20,9 @@ type PrincipalNetworkDomain interface {
 	RemoveSbi(uuid.UUID) error
 	AddDevice(interface{}) error
 	GetDevice(uuid uuid.UUID) (ygot.GoStruct, error)
-	ChangeOND(uuid uuid.UUID, operation interface{}, path string, value ...string) error
 	RemoveDevice(uuid.UUID) error
+	Devices() []uuid.UUID
+	ChangeOND(uuid uuid.UUID, operation interface{}, path string, value ...string) error
 	Request(uuid.UUID, string) error
 	RequestAll(string) error
 	GetName() string
@@ -30,21 +31,10 @@ type PrincipalNetworkDomain interface {
 	ContainsDevice(uuid.UUID) bool
 	GetSBIs() interface{}
 	ID() uuid.UUID
-	ListPending() []uuid.UUID
-	Pending(uuid.UUID) (interface{}, error)
-	ListCommitted() []uuid.UUID
-	Committed(uuid.UUID) (interface{}, error)
-}
-
-type pndImplementation struct {
-	name             string
-	description      string
-	sbic             sbiStore
-	devices          deviceStore
-	pendingChanges   changeStore
-	committedChanges changeStore
-	confirmedChanges changeStore
-	id               uuid.UUID
+	Pending() []uuid.UUID
+	Committed() []uuid.UUID
+	Commit(uuid.UUID) error
+	Confirm(uuid.UUID) error
 }
 
 // NewPND creates a Principle Network Domain
@@ -58,6 +48,7 @@ func NewPND(name, description string, id uuid.UUID, sbi SouthboundInterface) (Pr
 		committedChanges: changeStore{store{}},
 		confirmedChanges: changeStore{store{}},
 		id:               id,
+		errChans:         make(map[uuid.UUID]chan error),
 	}
 	if err := pnd.sbic.add(sbi); err != nil {
 		return nil, &ErrAlreadyExists{item: sbi}
@@ -65,26 +56,72 @@ func NewPND(name, description string, id uuid.UUID, sbi SouthboundInterface) (Pr
 	return pnd, nil
 }
 
-func (pnd *pndImplementation) ListPending() []uuid.UUID {
+type pndImplementation struct {
+	name             string
+	description      string
+	sbic             sbiStore
+	devices          deviceStore
+	pendingChanges   changeStore
+	committedChanges changeStore
+	confirmedChanges changeStore
+	id               uuid.UUID
+	errChans         map[uuid.UUID]chan error
+}
+
+func (pnd *pndImplementation) Pending() []uuid.UUID {
 	return pnd.pendingChanges.UUIDs()
 }
 
-func (pnd *pndImplementation) ListCommitted() []uuid.UUID {
+func (pnd *pndImplementation) Committed() []uuid.UUID {
 	return pnd.committedChanges.UUIDs()
 }
 
-func (pnd *pndImplementation)Pending(id uuid.UUID) (interface{}, error) {
-	return pnd.pendingChanges.get(id)
+func (pnd *pndImplementation) Commit(u uuid.UUID) error {
+	change, err := pnd.pendingChanges.get(u)
+	if err != nil {
+		return err
+	}
+	if err := change.Commit(); err != nil {
+		return err
+	}
+	go func() {
+		for {
+			select {
+			case err := <-pnd.errChans[u]:
+				handleRollbackError(change.ID(), err)
+			case <-change.Done:
+			}
+		}
+	}()
+	if err := pnd.committedChanges.add(change); err != nil {
+		return err
+	}
+	return pnd.pendingChanges.delete(u)
 }
 
-func (pnd *pndImplementation)Committed(id uuid.UUID) (interface{}, error) {
-	return pnd.committedChanges.get(id)
+func (pnd *pndImplementation) Confirm(u uuid.UUID) error {
+	change, err := pnd.committedChanges.get(u)
+	if err != nil {
+		return err
+	}
+	if err := change.Confirm(); err != nil {
+		return err
+	}
+	if err := pnd.confirmedChanges.add(change); err != nil {
+		return err
+	}
+	close(pnd.errChans[u])
+	return pnd.committedChanges.delete(u)
 }
 
 func (pnd *pndImplementation) ID() uuid.UUID {
 	return pnd.id
 }
 
+func (pnd *pndImplementation) Devices() []uuid.UUID {
+	return pnd.devices.UUIDs()
+}
+
 // GetName returns the name of the PND
 func (pnd *pndImplementation) GetName() string {
 	return pnd.name
@@ -243,6 +280,13 @@ func (pnd *pndImplementation) ChangeOND(uuid uuid.UUID, operation interface{}, p
 		return err
 	}
 
+	if len(value) > 1 {
+		return &ErrInvalidParameters{
+			f: pnd.ChangeOND,
+			r: value,
+		}
+	}
+
 	switch operation {
 	case TRANSPORT_UPDATE, TRANSPORT_REPLACE:
 		typedValue := gnmi.TypedValue(value[0])
@@ -262,7 +306,14 @@ func (pnd *pndImplementation) ChangeOND(uuid uuid.UUID, operation interface{}, p
 		return d.Transport.Set(ctx, state, change)
 	}
 
-	change := NewChange(uuid, d.GoStruct, cpy, callback)
+	errChan := make(chan error)
+	change := NewChange(uuid, d.GoStruct, cpy, callback, errChan)
+	pnd.errChans[change.ID()] = errChan
 
 	return pnd.pendingChanges.add(change)
 }
+
+func handleRollbackError(id uuid.UUID, err error) {
+	log.Error(err)
+	// TODO: Notion of invalid state needed.
+}

nucleus/principalNetworkDomain_test.go

@@ -524,16 +524,7 @@ func Test_pndImplementation_RequestAll(t *testing.T) {
 }
 
 func Test_pndImplementation_ChangeOND(t *testing.T) {
-	t.Fail()
 	type fields struct {
-		name             string
-		description      string
-		sbic             sbiStore
-		devices          deviceStore
-		pendingChanges   changeStore
-		committedChanges changeStore
-		confirmedChanges changeStore
-		id               uuid.UUID
 	}
 	type args struct {
 		uuid      uuid.UUID
@@ -547,23 +538,98 @@ func Test_pndImplementation_ChangeOND(t *testing.T) {
 		args    args
 		wantErr bool
 	}{
-		// TODO: Add test cases.
+		{
+			name:   "update",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: TRANSPORT_UPDATE,
+				path:      "/system/config/hostname",
+				value:     []string{"ceos3000"},
+			},
+			wantErr: false,
+		},
+		{
+			name:   "replace",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: TRANSPORT_REPLACE,
+				path:      "/system/config/hostname",
+				value:     []string{"ceos3000"},
+			},
+			wantErr: false,
+		},
+		{
+			name:   "delete",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: TRANSPORT_DELETE,
+				path:      "/system/config/hostname",
+			},
+			wantErr: false,
+		},
+		{
+			name:   "delete w/args",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: TRANSPORT_DELETE,
+				path:      "/system/config/hostname",
+				value:     []string{"ceos3000"},
+			},
+			wantErr: false,
+		},
+
+		// Negative test cases
+		{
+			name:   "invalid operation",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: "INVALID",
+			},
+			wantErr: true,
+		},
+		{
+			name:   "invalid arg count",
+			fields: fields{},
+			args: args{
+				uuid:      mdid,
+				operation: TRANSPORT_UPDATE,
+				path:      "/system/config/hostname",
+				value:     []string{"ceos3000", "ceos3001"},
+			},
+			wantErr: true,
+		},
+		{
+			name:   "device not found",
+			fields: fields{},
+			args: args{
+				uuid:      did,
+				operation: TRANSPORT_UPDATE,
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			pnd := &pndImplementation{
-				name:             tt.fields.name,
-				description:      tt.fields.description,
-				sbic:             tt.fields.sbic,
-				devices:          tt.fields.devices,
-				pendingChanges:   tt.fields.pendingChanges,
-				committedChanges: tt.fields.committedChanges,
-				confirmedChanges: tt.fields.confirmedChanges,
-				id:               tt.fields.id,
-			}
-			if err := pnd.ChangeOND(tt.args.uuid, tt.args.operation, tt.args.path, tt.args.value...); (err != nil) != tt.wantErr {
+			p := newPnd()
+			d := mockDevice()
+			if err := p.AddDevice(&d); err != nil {
+				t.Error(err)
+				return
+			}
+			if err := p.ChangeOND(tt.args.uuid, tt.args.operation, tt.args.path, tt.args.value...); (err != nil) != tt.wantErr {
 				t.Errorf("ChangeOND() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !tt.wantErr {
+				if len(p.pendingChanges.store) != 1 {
+					t.Errorf("ChangeOND() unexpected change count. got %v, want 1", len(p.pendingChanges.store))
+				}
 			}
 		})
 	}
-}
\ No newline at end of file
+}