Add tests to PPS Source Activate function (#395)

Summary:
Pull Request resolved: #395

WHAT?

Adds tests to ActivatePPSSource function

WHY?

Ensuring functional correctnes, ensuring edge cases work and making the code more maintainable for the future

Considerations:

That's a lot of code!
 - Yeah, but it is necessary to do the rigging for testing the syscalls

Why do you have matchBy with return true?
 - To dereference the pointer of the struct passed as argument, and then assert it with require.Equals, which gives nice comparison logs

Reviewed By: leoleovich

Differential Revision: D62447639

phc/phc.go

@@ -34,8 +34,8 @@ const DefaultMaxClockFreqPPB = 500000.0
 const (
 	ptpPeroutDutyCycle   = (1 << 1)
 	ptpPeroutPhase       = (1 << 2)
-	defaultTs2PhcChannel = uint32(0)
-	defaultTs2PhcIndex   = uint32(0)
+	defaultTs2PhcChannel = 0
+	defaultTs2PhcIndex   = 0
 	defaultPulseWidth    = int32(500000000)
 	// should default to 0 if config specified. Otherwise -1 (ignore phase)
 	defaultPeroutPhase = int32(-1) //nolint:all
@@ -155,6 +155,14 @@ func Time(iface string, method TimeMethod) (time.Time, error) {
 // Device represents a PHC device
 type Device os.File
 
+// DeviceController defines a subset of functions to interact with a phc device. Enables mocking.
+type DeviceController interface {
+	Time() (time.Time, error)
+	setPinFunc(index uint, pf PinFunc, ch uint) error
+	setPTPPerout(req PTPPeroutRequest) error
+	File() *os.File
+}
+
 // FromFile returns a *Device corresponding to an *os.File
 func FromFile(file *os.File) *Device { return (*Device)(file) }
 
@@ -284,6 +292,10 @@ func (dev *Device) MaxFreqAdjPPB() (maxFreq float64, err error) {
 	return caps.maxAdj(), nil
 }
 
+func (dev *Device) setPTPPerout(req PTPPeroutRequest) error {
+	return dev.ioctl(ioctlPTPPeroutRequest2, unsafe.Pointer(&req))
+}
+
 // FreqPPB reads PHC device frequency in PPB (parts per billion)
 func (dev *Device) FreqPPB() (freqPPB float64, err error) { return freqPPBFromDevice(dev) }
 
@@ -294,15 +306,11 @@ func (dev *Device) AdjFreq(freqPPB float64) error { return clockAdjFreq(dev, fre
 func (dev *Device) Step(step time.Duration) error { return clockStep(dev, step) }
 
 // ActivatePPSSource configures the PHC device to be a PPS timestamp source
-func ActivatePPSSource(dev Device) error {
+func ActivatePPSSource(dev DeviceController) error {
 	// Initialize the PTPPeroutRequest struct
 	peroutRequest := PTPPeroutRequest{}
 
-	err := dev.ioctl(iocPinSetfunc2, unsafe.Pointer(&rawPinDesc{
-		Index: defaultTs2PhcIndex,
-		Chan:  defaultTs2PhcChannel,
-		Func:  uint32(PinFuncPerOut),
-	}))
+	err := dev.setPinFunc(defaultTs2PhcIndex, PinFuncPerOut, defaultTs2PhcChannel)
 
 	if err != nil {
 		log.Errorf("Failed to set PPS Perout on pin index %d, channel %d, PHC %s. Error: %s. Continuing bravely on...",
@@ -329,12 +337,12 @@ func ActivatePPSSource(dev Device) error {
 	// TODO: reintroduce peroutPhase != -1 condition once peroutPhase is configurable
 	peroutRequest.StartOrPhase = PTPClockTime{Sec: int64(ts.Second() + ppsStartDelay), NSec: 0}
 
-	err = dev.ioctl(ioctlPTPPeroutRequest2, unsafe.Pointer(&peroutRequest))
+	err = dev.setPTPPerout(peroutRequest)
 
 	if err != nil {
 		log.Debugf("retrying PTP_PEROUT_REQUEST2 with DUTY_CYCLE flag unset for backwards compatibility")
 		peroutRequest.Flags &^= ptpPeroutDutyCycle
-		err = dev.ioctl(ioctlPTPPeroutRequest2, unsafe.Pointer(&peroutRequest))
+		err = dev.setPTPPerout(peroutRequest)
 		return err
 	}
 

phc/phc_test.go

@@ -17,11 +17,39 @@ limitations under the License.
 package phc
 
 import (
+	"fmt"
+	"os"
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
+type deviceControllerMock struct {
+	mock.Mock
+}
+
+func (_m *deviceControllerMock) Time() (time.Time, error) {
+	ret := _m.Called()
+	return ret.Get(0).(time.Time), ret.Error(1)
+}
+
+func (_m *deviceControllerMock) setPinFunc(index uint, pf PinFunc, ch uint) error {
+	ret := _m.Called(index, pf, ch)
+	return ret.Error(0)
+}
+
+func (_m *deviceControllerMock) setPTPPerout(req PTPPeroutRequest) error {
+	ret := _m.Called(req)
+	return ret.Error(0)
+}
+
+func (_m *deviceControllerMock) File() *os.File {
+	ret := _m.Called()
+	return ret.Get(0).(*os.File)
+}
+
 func TestIfaceInfoToPHCDevice(t *testing.T) {
 	info := &EthtoolTSinfo{
 		PHCIndex: 0,
@@ -52,3 +80,94 @@ func TestMaxAdjFreq(t *testing.T) {
 	got = caps.maxAdj()
 	require.InEpsilon(t, 500000.0, got, 0.00001)
 }
+
+func TestActivatePPSSource(t *testing.T) {
+	// Prepare
+	mockDevice := new(deviceControllerMock)
+
+	// Should set default pin to PPS
+	mockDevice.On("setPinFunc", uint(0), PinFuncPerOut, uint(0)).Return(nil).Once()
+
+	// Should call Time once
+	mockDevice.On("Time").Return(time.Unix(824635825488, 1397965136), nil).Once()
+
+	// Should issue ioctlPTPPeroutRequest2
+	expectedPeroutRequest := PTPPeroutRequest{
+		Index:        uint32(0),
+		Flags:        uint32(0x2),
+		StartOrPhase: PTPClockTime{Sec: 51},
+		Period:       PTPClockTime{Sec: 1},
+		On:           PTPClockTime{NSec: 500000000},
+	}
+	mockDevice.On("setPTPPerout", expectedPeroutRequest).Return(nil).Once()
+
+	// Act
+	err := ActivatePPSSource(mockDevice)
+
+	// Assert calls
+	require.NoError(t, err)
+	mockDevice.AssertExpectations(t)
+}
+
+func TestActivatePPSSourceIgnoreSetPinFailure(t *testing.T) {
+	// Prepare
+	mockDevice := new(deviceControllerMock)
+	mockDevice.On("File").Return(os.NewFile(3, "mock_file"))
+	mockDevice.On("Time").Return(time.Unix(824635825488, 1397965136), nil)
+
+	// If ioctl set pin fails, we continue bravely on...
+	mockDevice.On("setPinFunc", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("error")).Once()
+	mockDevice.On("setPTPPerout", mock.Anything).Return(nil).Once()
+
+	// Act
+	err := ActivatePPSSource(mockDevice)
+
+	// Assert calls
+	mockDevice.AssertExpectations(t)
+	require.NoError(t, err)
+}
+
+func TestActivatePPSSourceSetPTPPeroutFailure(t *testing.T) {
+	// Prepare
+	mockDevice := new(deviceControllerMock)
+	mockDevice.On("File").Return(os.NewFile(3, "mock_file"))
+	mockDevice.On("Time").Return(time.Unix(824635825488, 1397965136), nil)
+
+	mockDevice.On("setPinFunc", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("error")).Once()
+	// If first attempt to set PTPPerout fails
+	mockDevice.On("setPTPPerout", mock.Anything).Return(fmt.Errorf("error")).Once()
+
+	// Should retry setPTPPerout with backward compatible flag
+	expectedPeroutRequest := PTPPeroutRequest{
+		Index:        uint32(0),
+		Flags:        uint32(0x0),
+		StartOrPhase: PTPClockTime{Sec: 51},
+		Period:       PTPClockTime{Sec: 1},
+		On:           PTPClockTime{NSec: 500000000},
+	}
+	mockDevice.On("setPTPPerout", expectedPeroutRequest).Return(nil).Once()
+
+	// Act
+	err := ActivatePPSSource(mockDevice)
+
+	// Assert
+	mockDevice.AssertExpectations(t)
+	require.NoError(t, err)
+}
+
+func TestActivatePPSSourceSetPTPPeroutDoubleFailure(t *testing.T) {
+	// Prepare
+	mockDevice := new(deviceControllerMock)
+	mockDevice.On("File").Return(os.NewFile(3, "mock_file"))
+	mockDevice.On("Time").Return(time.Unix(824635825488, 1397965136), nil)
+	mockDevice.On("setPinFunc", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("error")).Once()
+	mockDevice.On("setPTPPerout", mock.Anything).Return(fmt.Errorf("error")).Once()
+	mockDevice.On("setPTPPerout", mock.Anything).Return(fmt.Errorf("error")).Once()
+
+	// Act
+	err := ActivatePPSSource(mockDevice)
+
+	// Assert
+	mockDevice.AssertExpectations(t)
+	require.Error(t, err)
+}