test: pid: zone

Add unit-tests for the PID zone module.
Add zone_mock.

Tested: Ran on quanta-q71l board and it behaved as expected.

Change-Id: I51185b2d2daacea6ffb687e8f38c4fe2b2a1bed3
Signed-off-by: Patrick Venture <venture@google.com>

13 files changed, 486 insertions, 37 deletions

diff --git a/pid/controller.hpp b/pid/controller.hpp
index d02f9cc..fa89ad1 100644
--- a/pid/controller.hpp
+++ b/pid/controller.hpp
@@ -6,7 +6,7 @@
 #include "fan.hpp"
 #include "ec/pid.hpp"
 
-class PIDZone;
+class ZoneInterface;
 
 /*
  * Base class for PID controllers.  Each PID that implements this needs to
@@ -15,7 +15,7 @@ class PIDZone;
 class PIDController
 {
     public:
-        PIDController(const std::string& id, PIDZone* owner)
+        PIDController(const std::string& id, ZoneInterface* owner)
             : _owner(owner),
               _id(id)
         { }
@@ -47,7 +47,7 @@ class PIDController
         }
 
     protected:
-        PIDZone* _owner;
+        ZoneInterface* _owner;
 
     private:
         // parameters
diff --git a/pid/fancontroller.cpp b/pid/fancontroller.cpp
index 340c6d0..c2dee74 100644
--- a/pid/fancontroller.cpp
+++ b/pid/fancontroller.cpp
@@ -22,7 +22,7 @@
 #include "zone.hpp"
 
 std::unique_ptr<PIDController> FanController::CreateFanPid(
-    PIDZone* owner,
+    ZoneInterface* owner,
     const std::string& id,
     std::vector<std::string>& inputs,
     ec::pidinfo initial)
@@ -133,7 +133,7 @@ void FanController::output_proc(float value)
     // PidSensorMap for writing.
     for (auto& it : _inputs)
     {
-        auto& sensor = _owner->getSensor(it);
+        auto sensor = _owner->getSensor(it);
         sensor->write(static_cast<double>(percent));
     }
 
diff --git a/pid/fancontroller.hpp b/pid/fancontroller.hpp
index b627915..b9b015c 100644
--- a/pid/fancontroller.hpp
+++ b/pid/fancontroller.hpp
@@ -18,14 +18,14 @@ class FanController : public PIDController
 {
     public:
         static std::unique_ptr<PIDController> CreateFanPid(
-            PIDZone* owner,
+            ZoneInterface* owner,
             const std::string& id,
             std::vector<std::string>& inputs,
             ec::pidinfo initial);
 
         FanController(const std::string& id,
                       std::vector<std::string>& inputs,
-                      PIDZone* owner)
+                      ZoneInterface* owner)
             : PIDController(id, owner),
               _inputs(inputs),
               _direction(FanSpeedDirection::NEUTRAL)
diff --git a/pid/thermalcontroller.cpp b/pid/thermalcontroller.cpp
index 61e5ca9..dee2d4e 100644
--- a/pid/thermalcontroller.cpp
+++ b/pid/thermalcontroller.cpp
@@ -20,7 +20,7 @@
 
 
 std::unique_ptr<PIDController> ThermalController::CreateThermalPid(
-    PIDZone* owner,
+    ZoneInterface* owner,
     const std::string& id,
     std::vector<std::string>& inputs,
     float setpoint,
diff --git a/pid/thermalcontroller.hpp b/pid/thermalcontroller.hpp
index dbfcfc9..32d4a7f 100644
--- a/pid/thermalcontroller.hpp
+++ b/pid/thermalcontroller.hpp
@@ -16,7 +16,7 @@ class ThermalController : public PIDController
 {
     public:
         static std::unique_ptr<PIDController> CreateThermalPid(
-            PIDZone* owner,
+            ZoneInterface* owner,
             const std::string& id,
             std::vector<std::string>& inputs,
             float setpoint,
@@ -24,7 +24,7 @@ class ThermalController : public PIDController
 
         ThermalController(const std::string& id,
                           std::vector<std::string>& inputs,
-                          PIDZone* owner)
+                          ZoneInterface* owner)
             : PIDController(id, owner),
               _inputs(inputs)
         { }
diff --git a/pid/zone.cpp b/pid/zone.cpp
index 26bff6a..fdf16fc 100644
--- a/pid/zone.cpp
+++ b/pid/zone.cpp
@@ -208,7 +208,7 @@ void PIDZone::updateFanTelemetry(void)
 
     for (auto& f : _fanInputs)
     {
-        auto& sensor = _mgr.getSensor(f);
+        auto sensor = _mgr.getSensor(f);
         ReadReturn r = sensor->read();
         _cachedValuesByName[f] = r.value;
 
@@ -240,7 +240,7 @@ void PIDZone::updateSensors(void)
 
     for (auto& t : _thermalInputs)
     {
-        auto& sensor = _mgr.getSensor(t);
+        auto sensor = _mgr.getSensor(t);
         ReadReturn r = sensor->read();
         int64_t timeout = sensor->GetTimeout();
 
@@ -316,7 +316,7 @@ void PIDZone::process_thermals(void)
     }
 }
 
-const std::unique_ptr<Sensor>& PIDZone::getSensor(std::string name)
+Sensor* PIDZone::getSensor(std::string name)
 {
     return _mgr.getSensor(name);
 }
diff --git a/pid/zone.hpp b/pid/zone.hpp
index f6ee5e8..1934a66 100644
--- a/pid/zone.hpp
+++ b/pid/zone.hpp
@@ -23,12 +23,25 @@ using ModeInterface =
     sdbusplus::xyz::openbmc_project::Control::server::Mode;
 using ModeObject = ServerObject<ModeInterface>;
 
+class ZoneInterface
+{
+    public:
+        virtual ~ZoneInterface() = default;
+
+        virtual double getCachedValue(const std::string& name) = 0;
+        virtual void addRPMSetPoint(float setpoint) = 0;
+        virtual float getMaxRPMRequest() const = 0;
+        virtual bool getFailSafeMode() const = 0;
+        virtual float getFailSafePercent() const = 0;
+        virtual Sensor* getSensor(std::string name) = 0;
+};
+
 /*
  * The PIDZone inherits from the Mode object so that it can listen for control
  * mode changes.  It primarily holds all PID loops and holds the sensor value
  * cache that's used per iteration of the PID loops.
  */
-class PIDZone : public ModeObject
+class PIDZone : public ZoneInterface, public ModeObject
 {
     public:
         PIDZone(int64_t zone,
@@ -50,21 +63,21 @@ class PIDZone : public ModeObject
 #endif
         }
 
-        float getMaxRPMRequest(void) const;
+        float getMaxRPMRequest(void) const override;
         bool getManualMode(void) const;
 
         /* Could put lock around this since it's accessed from two threads, but
          * only one reader/one writer.
          */
         void setManualMode(bool mode);
-        bool getFailSafeMode(void) const;
+        bool getFailSafeMode(void) const override;
         int64_t getZoneId(void) const;
-        void addRPMSetPoint(float setpoint);
+        void addRPMSetPoint(float setpoint) override;
         void clearRPMSetPoints(void);
-        float getFailSafePercent(void) const;
+        float getFailSafePercent(void) const override;
         float getMinThermalRpmSetPt(void) const;
 
-        const std::unique_ptr<Sensor>& getSensor(std::string name);
+        Sensor* getSensor(std::string name) override;
         void determineMaxRPMRequest(void);
         void updateFanTelemetry(void);
         void updateSensors(void);
@@ -75,7 +88,7 @@ class PIDZone : public ModeObject
 
         void addFanPID(std::unique_ptr<PIDController> pid);
         void addThermalPID(std::unique_ptr<PIDController> pid);
-        double getCachedValue(const std::string& name);
+        double getCachedValue(const std::string& name) override;
         void addFanInput(std::string fan);
         void addThermalInput(std::string therm);
 
diff --git a/sensors/manager.hpp b/sensors/manager.hpp
index d37787a..ebf4962 100644
--- a/sensors/manager.hpp
+++ b/sensors/manager.hpp
@@ -46,9 +46,9 @@ class SensorManager
             std::unique_ptr<Sensor> sensor);
 
         // TODO(venture): Should implement read/write by name.
-        const std::unique_ptr<Sensor>& getSensor(const std::string& name) const
+        Sensor* getSensor(const std::string& name) const
         {
-            return _sensorMap.at(name);
+            return _sensorMap.at(name).get();
         }
 
         sdbusplus::bus::bus& getPassiveBus(void)
diff --git a/test/Makefile.am b/test/Makefile.am
index 7fdbaf5..ec40421 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -12,7 +12,7 @@ AM_LDFLAGS = \
 
 # Run all 'check' test programs
 check_PROGRAMS = sensor_manager_unittest sensor_pluggable_unittest \
- sensor_host_unittest util_unittest
+ sensor_host_unittest util_unittest pid_zone_unittest
 TESTS = $(check_PROGRAMS)
 
 # Until libconfig is mocked out or replaced, include it.
@@ -27,3 +27,8 @@ sensor_host_unittest_LDADD = $(top_builddir)/sensors/host.o
 
 util_unittest_SOURCES = util_unittest.cpp
 util_unittest_LDADD = $(top_builddir)/util.o
+
+pid_zone_unittest_SOURCES = pid_zone_unittest.cpp
+pid_zone_unittest_LDADD = $(top_builddir)/pid/ec/pid.o \
+ $(top_builddir)/sensors/manager.o $(top_builddir)/pid/controller.o \
+ $(top_builddir)/pid/zone.o
diff --git a/test/helpers.hpp b/test/helpers.hpp
index 931a39e..74c969f 100644
--- a/test/helpers.hpp
+++ b/test/helpers.hpp
@@ -49,17 +49,7 @@ void SetupDbusObject(sdbusplus::SdBusMock *sdbus_mock, bool defer,
             .WillOnce(Return(0));
     }
 
-    if (properties.empty())
-    {
-        // We always expect one, but in this case we're not concerned with the
-        // output.  If there is more than one property update, we should care
-        // and the test writer can add the code to trigger the below case.
-        EXPECT_CALL(*sdbus_mock,
-                    sd_bus_emit_properties_changed_strv(IsNull(), StrEq(path),
-                                                        StrEq(intf), NotNull()))
-            .WillOnce(Return(0));
-    }
-    else
+    if (!properties.empty())
     {
         (*index) = 0;
         EXPECT_CALL(*sdbus_mock,
diff --git a/test/pid_zone_unittest.cpp b/test/pid_zone_unittest.cpp
new file mode 100644
index 0000000..c511b3c
--- /dev/null
+++ b/test/pid_zone_unittest.cpp
@@ -0,0 +1,422 @@
+#include "pid/zone.hpp"
+
+#include <chrono>
+#include <cstring>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <sdbusplus/test/sdbus_mock.hpp>
+#include <vector>
+
+#include "pid/ec/pid.hpp"
+#include "sensors/manager.hpp"
+#include "test/controller_mock.hpp"
+#include "test/sensor_mock.hpp"
+#include "test/helpers.hpp"
+
+using ::testing::IsNull;
+using ::testing::Return;
+using ::testing::StrEq;
+using ::testing::_;
+
+static std::string modeInterface = "xyz.openbmc_project.Control.Mode";
+
+namespace {
+
+TEST(PidZoneConstructorTest, BoringConstructorTest) {
+    // Build a PID Zone.
+
+    sdbusplus::SdBusMock sdbus_mock_passive, sdbus_mock_host, sdbus_mock_mode;
+    auto bus_mock_passive = sdbusplus::get_mocked_new(&sdbus_mock_passive);
+    auto bus_mock_host = sdbusplus::get_mocked_new(&sdbus_mock_host);
+    auto bus_mock_mode = sdbusplus::get_mocked_new(&sdbus_mock_mode);
+
+    EXPECT_CALL(sdbus_mock_host,
+                sd_bus_add_object_manager(
+                    IsNull(),
+                    _,
+                    StrEq("/xyz/openbmc_project/extsensors")))
+        .WillOnce(Return(0));
+
+    SensorManager m(std::move(bus_mock_passive),
+                     std::move(bus_mock_host));
+
+    bool defer = true;
+    const char *objPath = "/path/";
+    int64_t zone = 1;
+    float minThermalRpm = 1000.0;
+    float failSafePercent = 0.75;
+
+    int i;
+    std::vector<std::string> properties;
+    SetupDbusObject(&sdbus_mock_mode, defer, objPath, modeInterface,
+                    properties, &i);
+
+    PIDZone p(zone, minThermalRpm, failSafePercent, m, bus_mock_mode, objPath,
+              defer);
+    // Success.
+}
+
+}
+
+class PidZoneTest : public ::testing::Test {
+    protected:
+        PidZoneTest()
+        : property_index(),
+          properties(),
+          sdbus_mock_passive(),
+          sdbus_mock_host(),
+          sdbus_mock_mode()
+        {
+            EXPECT_CALL(sdbus_mock_host,
+                sd_bus_add_object_manager(
+                    IsNull(),
+                    _,
+                    StrEq("/xyz/openbmc_project/extsensors")))
+                .WillOnce(Return(0));
+
+            auto bus_mock_passive =
+                sdbusplus::get_mocked_new(&sdbus_mock_passive);
+            auto bus_mock_host = sdbusplus::get_mocked_new(&sdbus_mock_host);
+            auto bus_mock_mode = sdbusplus::get_mocked_new(&sdbus_mock_mode);
+
+            // Compiler weirdly not happy about just instantiating mgr(...);
+            SensorManager m(std::move(bus_mock_passive),
+                            std::move(bus_mock_host));
+            mgr = std::move(m);
+
+            SetupDbusObject(&sdbus_mock_mode, defer, objPath, modeInterface,
+                            properties, &property_index);
+
+            zone = std::make_unique<PIDZone>(zoneId, minThermalRpm,
+                                             failSafePercent, mgr,
+                                             bus_mock_mode, objPath, defer);
+        }
+
+        // unused
+        int property_index;
+        std::vector<std::string> properties;
+
+        sdbusplus::SdBusMock sdbus_mock_passive;
+        sdbusplus::SdBusMock sdbus_mock_host;
+        sdbusplus::SdBusMock sdbus_mock_mode;
+        int64_t zoneId = 1;
+        float minThermalRpm = 1000.0;
+        float failSafePercent = 0.75;
+        bool defer = true;
+        const char *objPath = "/path/";
+        SensorManager mgr;
+
+        std::unique_ptr<PIDZone> zone;
+};
+
+TEST_F(PidZoneTest, GetZoneId_ReturnsExpected) {
+    // Verifies the zoneId returned is what we expect.
+
+    EXPECT_EQ(zoneId, zone->getZoneId());
+}
+
+TEST_F(PidZoneTest, GetAndSetManualModeTest_BehavesAsExpected) {
+    // Verifies that the zone starts in manual mode.  Verifies that one can set
+    // the mode.
+    EXPECT_FALSE(zone->getManualMode());
+
+    zone->setManualMode(true);
+    EXPECT_TRUE(zone->getManualMode());
+}
+
+TEST_F(PidZoneTest, RpmSetPoints_AddMaxClear_BehaveAsExpected) {
+    // Tests addRPMSetPoint, clearRPMSetPoints, determineMaxRPMRequest
+    // and getMinThermalRpmSetPt.
+
+    // At least one value must be above the minimum thermal setpoint used in
+    // the constructor otherwise it'll choose that value
+    std::vector<float> values = {100, 200, 300, 400, 500, 5000};
+    for (auto v : values)
+    {
+        zone->addRPMSetPoint(v);
+    }
+
+    // This will pull the maximum RPM setpoint request.
+    zone->determineMaxRPMRequest();
+    EXPECT_EQ(5000, zone->getMaxRPMRequest());
+
+    // Clear the values, so it'll choose the minimum thermal setpoint.
+    zone->clearRPMSetPoints();
+
+    // This will go through the RPM set point values and grab the maximum.
+    zone->determineMaxRPMRequest();
+    EXPECT_EQ(zone->getMinThermalRpmSetPt(), zone->getMaxRPMRequest());
+}
+
+TEST_F(PidZoneTest, RpmSetPoints_AddBelowMinimum_BehavesAsExpected) {
+    // Tests adding several RPM setpoints, however, they're all lower than the
+    // configured minimal thermal set-point RPM value.
+
+    std::vector<float> values = {100, 200, 300, 400, 500};
+    for (auto v : values)
+    {
+        zone->addRPMSetPoint(v);
+    }
+
+    // This will pull the maximum RPM setpoint request.
+    zone->determineMaxRPMRequest();
+
+    // Verifies the value returned in the minimal thermal rpm set point.
+    EXPECT_EQ(zone->getMinThermalRpmSetPt(), zone->getMaxRPMRequest());
+}
+
+TEST_F(PidZoneTest, GetFailSafePercent_ReturnsExpected) {
+    // Verify the value used to create the object is stored.
+    EXPECT_EQ(failSafePercent, zone->getFailSafePercent());
+}
+
+TEST_F(PidZoneTest, ThermalInputs_FailsafeToValid_ReadsSensors) {
+    // This test will add a couple thermal inputs, and verify that the zone
+    // initializes into failsafe mode, and will read each sensor.
+
+    std::string name1 = "temp1";
+    int64_t timeout = 1;
+
+    std::unique_ptr<Sensor> sensor1 =
+        std::make_unique<SensorMock>(name1, timeout);
+    SensorMock *sensor_ptr1 = reinterpret_cast<SensorMock*>(sensor1.get());
+
+    std::string name2 = "temp2";
+    std::unique_ptr<Sensor> sensor2 =
+        std::make_unique<SensorMock>(name2, timeout);
+    SensorMock *sensor_ptr2 = reinterpret_cast<SensorMock*>(sensor2.get());
+
+    std::string type = "unchecked";
+    mgr.addSensor(type, name1, std::move(sensor1));
+    EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1);
+    mgr.addSensor(type, name2, std::move(sensor2));
+    EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2);
+
+    // Now that the sensors exist, add them to the zone.
+    zone->addThermalInput(name1);
+    zone->addThermalInput(name2);
+
+    // Initialize Zone
+    zone->initializeCache();
+
+    // Verify now in failsafe mode.
+    EXPECT_TRUE(zone->getFailSafeMode());
+
+    ReadReturn r1;
+    r1.value = 10.0;
+    r1.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1));
+
+    ReadReturn r2;
+    r2.value = 11.0;
+    r2.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2));
+
+    // Read the sensors, this will put the values into the cache.
+    zone->updateSensors();
+
+    // We should no longer be in failsafe mode.
+    EXPECT_FALSE(zone->getFailSafeMode());
+
+    EXPECT_EQ(r1.value, zone->getCachedValue(name1));
+    EXPECT_EQ(r2.value, zone->getCachedValue(name2));
+}
+
+TEST_F(PidZoneTest, FanInputTest_VerifiesFanValuesCached) {
+    // This will add a couple fan inputs, and verify the values are cached.
+
+    std::string name1 = "fan1";
+    int64_t timeout = 2;
+
+    std::unique_ptr<Sensor> sensor1 =
+        std::make_unique<SensorMock>(name1, timeout);
+    SensorMock *sensor_ptr1 = reinterpret_cast<SensorMock*>(sensor1.get());
+
+    std::string name2 = "fan2";
+    std::unique_ptr<Sensor> sensor2 =
+        std::make_unique<SensorMock>(name2, timeout);
+    SensorMock *sensor_ptr2 = reinterpret_cast<SensorMock*>(sensor2.get());
+
+    std::string type = "unchecked";
+    mgr.addSensor(type, name1, std::move(sensor1));
+    EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1);
+    mgr.addSensor(type, name2, std::move(sensor2));
+    EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2);
+
+    // Now that the sensors exist, add them to the zone.
+    zone->addFanInput(name1);
+    zone->addFanInput(name2);
+
+    // Initialize Zone
+    zone->initializeCache();
+
+    ReadReturn r1;
+    r1.value = 10.0;
+    r1.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1));
+
+    ReadReturn r2;
+    r2.value = 11.0;
+    r2.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2));
+
+    // Method under test will read through each fan sensor for the zone and
+    // cache the values.
+    zone->updateFanTelemetry();
+
+    EXPECT_EQ(r1.value, zone->getCachedValue(name1));
+    EXPECT_EQ(r2.value, zone->getCachedValue(name2));
+}
+
+TEST_F(PidZoneTest, ThermalInput_ValueTimeoutEntersFailSafeMode) {
+    // On the second updateSensors call, the updated timestamp will be beyond
+    // the timeout limit.
+
+    int64_t timeout = 1;
+
+    std::string name1 = "temp1";
+    std::unique_ptr<Sensor> sensor1 =
+        std::make_unique<SensorMock>(name1, timeout);
+    SensorMock *sensor_ptr1 = reinterpret_cast<SensorMock*>(sensor1.get());
+
+    std::string name2 = "temp2";
+    std::unique_ptr<Sensor> sensor2 =
+        std::make_unique<SensorMock>(name2, timeout);
+    SensorMock *sensor_ptr2 = reinterpret_cast<SensorMock*>(sensor2.get());
+
+    std::string type = "unchecked";
+    mgr.addSensor(type, name1, std::move(sensor1));
+    EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1);
+    mgr.addSensor(type, name2, std::move(sensor2));
+    EXPECT_EQ(mgr.getSensor(name2), sensor_ptr2);
+
+    zone->addThermalInput(name1);
+    zone->addThermalInput(name2);
+
+    // Initialize Zone
+    zone->initializeCache();
+
+    // Verify now in failsafe mode.
+    EXPECT_TRUE(zone->getFailSafeMode());
+
+    ReadReturn r1;
+    r1.value = 10.0;
+    r1.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1));
+
+    ReadReturn r2;
+    r2.value = 11.0;
+    r2.updated = std::chrono::high_resolution_clock::now();
+    EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2));
+
+    zone->updateSensors();
+    EXPECT_FALSE(zone->getFailSafeMode());
+
+    // Ok, so we're not in failsafe mode, so let's set updated to the past.
+    // sensor1 will have an updated field older than its timeout value, but
+    // sensor2 will be fine. :D
+    r1.updated -= std::chrono::seconds(3);
+    r2.updated = std::chrono::high_resolution_clock::now();
+
+    EXPECT_CALL(*sensor_ptr1, read()).WillOnce(Return(r1));
+    EXPECT_CALL(*sensor_ptr2, read()).WillOnce(Return(r2));
+
+    // Method under test will read each sensor.  One sensor's value is older
+    // than the timeout for that sensor and this triggers failsafe mode.
+    zone->updateSensors();
+    EXPECT_TRUE(zone->getFailSafeMode());
+}
+
+TEST_F(PidZoneTest, GetSensorTest_ReturnsExpected) {
+    // One can grab a sensor from the manager through the zone.
+
+    int64_t timeout = 1;
+
+    std::string name1 = "temp1";
+    std::unique_ptr<Sensor> sensor1 =
+        std::make_unique<SensorMock>(name1, timeout);
+    SensorMock *sensor_ptr1 = reinterpret_cast<SensorMock*>(sensor1.get());
+
+    std::string type = "unchecked";
+    mgr.addSensor(type, name1, std::move(sensor1));
+    EXPECT_EQ(mgr.getSensor(name1), sensor_ptr1);
+
+    zone->addThermalInput(name1);
+
+    // Verify method under test returns the pointer we expect.
+    EXPECT_EQ(mgr.getSensor(name1), zone->getSensor(name1));
+}
+
+TEST_F(PidZoneTest, AddThermalPIDTest_VerifiesThermalPIDsProcessed) {
+    // Tests adding a thermal PID controller to the zone, and verifies it's
+    // touched during processing.
+
+    std::unique_ptr<PIDController> tpid =
+        std::make_unique<ControllerMock>("thermal1", zone.get());
+    ControllerMock *tmock = reinterpret_cast<ControllerMock*>(tpid.get());
+
+    // Access the internal pid configuration to clear it out (unrelated to the
+    // test).
+    ec::pid_info_t* info = tpid->get_pid_info();
+    std::memset(info, 0x00, sizeof(ec::pid_info_t));
+
+    zone->addThermalPID(std::move(tpid));
+
+    EXPECT_CALL(*tmock, setpt_proc()).WillOnce(Return(10.0));
+    EXPECT_CALL(*tmock, input_proc()).WillOnce(Return(11.0));
+    EXPECT_CALL(*tmock, output_proc(_));
+
+    // Method under test will, for each thermal PID, call setpt, input, and
+    // output.
+    zone->process_thermals();
+}
+
+TEST_F(PidZoneTest, AddFanPIDTest_VerifiesFanPIDsProcessed) {
+    // Tests adding a fan PID controller to the zone, and verifies it's
+    // touched during processing.
+
+    std::unique_ptr<PIDController> tpid =
+        std::make_unique<ControllerMock>("fan1", zone.get());
+    ControllerMock *tmock = reinterpret_cast<ControllerMock*>(tpid.get());
+
+    // Access the internal pid configuration to clear it out (unrelated to the
+    // test).
+    ec::pid_info_t* info = tpid->get_pid_info();
+    std::memset(info, 0x00, sizeof(ec::pid_info_t));
+
+    zone->addFanPID(std::move(tpid));
+
+    EXPECT_CALL(*tmock, setpt_proc()).WillOnce(Return(10.0));
+    EXPECT_CALL(*tmock, input_proc()).WillOnce(Return(11.0));
+    EXPECT_CALL(*tmock, output_proc(_));
+
+    // Method under test will, for each fan PID, call setpt, input, and output.
+    zone->process_fans();
+}
+
+TEST_F(PidZoneTest, ManualModeDbusTest_VerifySetManualBehavesAsExpected) {
+    // The manual(bool) method is inherited from the dbus mode interface.
+
+    // Verifies that someone doesn't remove the internal call to the dbus
+    // object from which we're inheriting.
+    EXPECT_CALL(sdbus_mock_mode,
+                sd_bus_emit_properties_changed_strv(IsNull(), StrEq(objPath),
+                                                    StrEq(modeInterface),
+                                                    NotNull()))
+        .WillOnce(Invoke([&](sd_bus *bus, const char *path,
+                             const char *interface, char **names) {
+            EXPECT_STREQ("Manual", names[0]);
+            return 0;
+        }));
+
+    // Method under test will set the manual mode to true and broadcast this
+    // change on dbus.
+    zone->manual(true);
+    EXPECT_TRUE(zone->getManualMode());
+}
+
+TEST_F(PidZoneTest, FailsafeDbusTest_VerifiesReturnsExpected) {
+    // This property is implemented by us as read-only, such that trying to
+    // write to it will have no effect.
+    EXPECT_EQ(zone->failSafe(), zone->getFailSafeMode());
+}
diff --git a/test/sensor_manager_unittest.cpp b/test/sensor_manager_unittest.cpp
index b873886..eed26cd 100644
--- a/test/sensor_manager_unittest.cpp
+++ b/test/sensor_manager_unittest.cpp
@@ -23,7 +23,7 @@ TEST(SensorManagerTest, BoringConstructorTest) {
                     IsNull(),
                     _,
                     StrEq("/xyz/openbmc_project/extsensors")))
-    .WillOnce(Return(0));
+        .WillOnce(Return(0));
 
     SensorManager s(std::move(bus_mock_passive), std::move(bus_mock_host));
     // Success
@@ -43,7 +43,7 @@ TEST(SensorManagerTest, AddSensorInvalidTypeTest) {
                     IsNull(),
                     _,
                     StrEq("/xyz/openbmc_project/extsensors")))
-    .WillOnce(Return(0));
+        .WillOnce(Return(0));
 
     SensorManager s(std::move(bus_mock_passive), std::move(bus_mock_host));
 
@@ -55,5 +55,5 @@ TEST(SensorManagerTest, AddSensorInvalidTypeTest) {
     Sensor *sensor_ptr = sensor.get();
 
     s.addSensor(type, name, std::move(sensor));
-    EXPECT_EQ(s.getSensor(name).get(), sensor_ptr);
+    EXPECT_EQ(s.getSensor(name), sensor_ptr);
 }
diff --git a/test/zone_mock.hpp b/test/zone_mock.hpp
new file mode 100644
index 0000000..f08c68f
--- /dev/null
+++ b/test/zone_mock.hpp
@@ -0,0 +1,19 @@
+#pragma once
+
+#include <gmock/gmock.h>
+#include <string>
+
+#include "pid/zone.hpp"
+
+class ZoneMock : public ZoneInterface
+{
+    public:
+        virtual ~ZoneMock() = default;
+
+        MOCK_METHOD1(getCachedValue, double(const std::string&));
+        MOCK_METHOD1(addRPMSetPoint, void(float));
+        MOCK_CONST_METHOD0(getMaxRPMRequest, float());
+        MOCK_CONST_METHOD0(getFailSafeMode, bool());
+        MOCK_CONST_METHOD0(getFailSafePercent, float());
+        MOCK_METHOD1(getSensor, Sensor*(std::string));
+};