From b41cacc98f4ceb4a5ea98b6cd9dbf3c0034ae924 Mon Sep 17 00:00:00 2001 From: Cristiano Ruschel Marques Dias Date: Thu, 12 Sep 2024 09:47:34 -0700 Subject: [PATCH] Add tests to PPS Source Activate function (#395) Summary: Pull Request resolved: https://github.com/facebook/time/pull/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 | 28 ++++++++---- phc/phc_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 10 deletions(-) diff --git a/phc/phc.go b/phc/phc.go index 0ace9982..1d0f5e44 100644 --- a/phc/phc.go +++ b/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 } diff --git a/phc/phc_test.go b/phc/phc_test.go index 70c8b016..265bd8d9 100644 --- a/phc/phc_test.go +++ b/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) +}