[machine] Interrogating iOS device requires getting the OS Version.
Since the OS version ends up as part of the Dimensions for the machine,
like DeviceType, a failure to interrogate it can't be ignored.
This was found by the presence of Prometheus metrics that were
identical except for os, which in one metric was os="iOS" and
in the other was os="iOS-13.3.1".
Change-Id: I7592a2b7db90b8b46b4b4890400d6fe4f08bd750
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/609117
Reviewed-by: Erik Rose <erikrose@google.com>
Auto-Submit: Joe Gregorio <jcgregorio@google.com>
Commit-Queue: Joe Gregorio <jcgregorio@google.com>
diff --git a/machine/go/test_machine_monitor/machine/machine.go b/machine/go/test_machine_monitor/machine/machine.go
index 9abd77c..697a10b3 100644
--- a/machine/go/test_machine_monitor/machine/machine.go
+++ b/machine/go/test_machine_monitor/machine/machine.go
@@ -462,11 +462,13 @@
}
ret.DeviceType = deviceType
- if osVersion, err := m.ios.OSVersion(ctx); err != nil {
- sklog.Warningf("Failed to read iOS version, though we managed to read the device type: %s", err)
- } else {
- ret.OSVersion = osVersion
+ // Since osVersion ends up as part of the Dimensions for the machine, like
+ // DeviceType, a failure here can't be ignored.
+ osVersion, err := m.ios.OSVersion(ctx)
+ if err != nil {
+ return ret, skerr.Wrapf(err, "reading iOS version")
}
+ ret.OSVersion = osVersion
battery, err := m.ios.BatteryLevel(ctx)
if err != nil {
diff --git a/machine/go/test_machine_monitor/machine/machine_test.go b/machine/go/test_machine_monitor/machine/machine_test.go
index e5d3408..e2e4575 100644
--- a/machine/go/test_machine_monitor/machine/machine_test.go
+++ b/machine/go/test_machine_monitor/machine/machine_test.go
@@ -264,16 +264,7 @@
}, actual)
}
-// Just make sure it can get into tryInterrogatingIOSDevice(), and test success while we're at it.
-// Tests that call tryInterrogatingIOSDevice() directly cover the other cases.
-func TestInterrogate_IOSDeviceAttached_Success(t *testing.T) {
- ctx := executil.FakeTestsContext(
- "Test_FakeExe_IDeviceInfo_ReturnsDeviceType",
- "Test_FakeExe_IDeviceInfo_ReturnsOSVersion",
- "Test_FakeExe_IDeviceInfo_ReturnsGoodBatteryLevel",
- )
-
- timePlaceholder := time.Date(2021, time.September, 2, 2, 2, 2, 2, time.UTC)
+func goodIOSInterrogationResult(timePlaceholder time.Time) (*Machine, machine.Event) {
m := &Machine{
adb: adb.New(),
ios: ios.New(),
@@ -287,9 +278,7 @@
AttachedDevice: machine.AttachedDeviceIOS,
},
}
- actual, err := m.interrogate(ctx)
- require.NoError(t, err)
- assert.Equal(t, machine.Event{
+ expected := machine.Event{
EventType: machine.EventTypeRawState,
LaunchedSwarming: true,
RunningSwarmingTask: true,
@@ -303,23 +292,50 @@
DeviceType: iOSDeviceTypePlaceholder,
Battery: 33,
},
- }, actual)
+ }
+ return m, expected
}
-func TestTryInterrogatingIOSDevice_OSVersionAndBatteryFail_StillSucceeds(t *testing.T) {
+// Just make sure it can get into tryInterrogatingIOSDevice(), and test success while we're at it.
+// Tests that call tryInterrogatingIOSDevice() directly cover the other cases.
+func TestInterrogate_IOSDeviceAttached_Success(t *testing.T) {
+ ctx := executil.FakeTestsContext(
+ "Test_FakeExe_IDeviceInfo_ReturnsDeviceType",
+ "Test_FakeExe_IDeviceInfo_ReturnsOSVersion",
+ "Test_FakeExe_IDeviceInfo_ReturnsGoodBatteryLevel",
+ )
+
+ timePlaceholder := time.Date(2021, time.September, 2, 2, 2, 2, 2, time.UTC)
+ m, expected := goodIOSInterrogationResult(timePlaceholder)
+ actual, err := m.interrogate(ctx)
+ require.NoError(t, err)
+ assert.Equal(t, expected, actual)
+}
+
+func TestInterrogate_IOSDeviceAttachedButBatteryCallFails_StillSuccess(t *testing.T) {
+ ctx := executil.FakeTestsContext(
+ "Test_FakeExe_IDeviceInfo_ReturnsDeviceType",
+ "Test_FakeExe_IDeviceInfo_ReturnsOSVersion",
+ "Test_FakeExe_ExitCodeOne", // battery level fails
+ )
+
+ timePlaceholder := time.Date(2021, time.September, 2, 2, 2, 2, 2, time.UTC)
+ m, expected := goodIOSInterrogationResult(timePlaceholder)
+ expected.IOS.Battery = machine.BadBatteryLevel // Battery value should reflect failure.
+ actual, err := m.interrogate(ctx)
+ require.NoError(t, err)
+ assert.Equal(t, expected, actual)
+}
+
+func TestTryInterrogatingIOSDevice_OSVersionFails_Fails(t *testing.T) {
ctx := executil.FakeTestsContext(
"Test_FakeExe_IDeviceInfo_ReturnsDeviceType",
"Test_FakeExe_ExitCodeOne", // OS version check goes kaboom.
- "Test_FakeExe_ExitCodeOne", // battery level too
+ "Test_FakeExe_IDeviceInfo_ReturnsGoodBatteryLevel",
)
m := &Machine{ios: ios.New()}
- actual, err := m.tryInterrogatingIOSDevice(ctx)
- require.NoError(t, err)
- assert.Equal(t, machine.IOS{
- OSVersion: "",
- DeviceType: iOSDeviceTypePlaceholder,
- Battery: machine.BadBatteryLevel,
- }, actual)
+ _, err := m.tryInterrogatingIOSDevice(ctx)
+ require.Error(t, err)
}
func TestTryInterrogatingIOSDevice_DeviceTypeFails_DeviceConsideredUnattached(t *testing.T) {
