Improve unit tests.

src/app/tests/TestReadInteraction.cpp

@@ -2359,12 +2359,12 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
 #else
         // When EventList is not enabled, the packet boundaries shift and for the first
         // report for the list attribute we receive two of its items in the initial list,
-        // then 4 additional items.  For the second report we receive 0 items in
-        // the initial list followed by 6 additional items.
+        // then 4 additional items.  For the second report we receive 3 items in
+        // the initial list followed by 3 additional items.
         //
-        // Thus we should receive 29*2 + 4 + 6 = 68 attribute data when the eventlist
+        // Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist
         // attribute is not available.
-        constexpr size_t kExpectedAttributeResponse = 68;
+        constexpr size_t kExpectedAttributeResponse = 65;
 #endif
         NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse);
         NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12);

src/controller/tests/TestReadChunking.cpp

@@ -70,6 +70,8 @@ constexpr AttributeId kTestListAttribute = 6;
 constexpr AttributeId kTestBadAttribute =
     7; // Reading this attribute will return CHIP_ERROR_NO_MEMORY but nothing is actually encoded.
 
+constexpr int kListAttributeItems = 5;
+
 class TestReadChunking
 {
 public:
@@ -125,6 +127,116 @@ DECLARE_DYNAMIC_ENDPOINT(testEndpoint5, testEndpoint5Clusters);
 
 uint8_t sAnStringThatCanNeverFitIntoTheMTU[4096] = { 0 };
 
+// Buffered callback class that lets us count the number of attribute data IBs
+// we receive.  BufferedReadCallback has all its ReadClient::Callback bits
+// private, so we can't just inherit from it and call our super-class functions.
+class TestBufferedReadCallback : public ReadClient::Callback
+{
+public:
+    TestBufferedReadCallback(ReadClient::Callback & aNextCallback) : mBufferedCallback(aNextCallback) {}
+
+    // Workaround for all the methods on BufferedReadCallback being private.
+    ReadClient::Callback & NextCallback() { return *static_cast<ReadClient::Callback *>(&mBufferedCallback); }
+
+    void OnReportBegin() override { NextCallback().OnReportBegin(); }
+    void OnReportEnd() override { NextCallback().OnReportEnd(); }
+
+    void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override
+    {
+        if (apData)
+        {
+            ++mAttributeDataIBCount;
+
+            TLV::TLVReader reader(*apData);
+            do
+            {
+                if (reader.GetType() != TLV::TLVType::kTLVType_Array)
+                {
+                    // Not a list.
+                    break;
+                }
+
+                TLV::TLVType containerType;
+                CHIP_ERROR err = reader.EnterContainer(containerType);
+                if (err != CHIP_NO_ERROR)
+                {
+                    mDecodingFailed = true;
+                    break;
+                }
+
+                err = reader.Next();
+                if (err == CHIP_END_OF_TLV)
+                {
+                    mSawEmptyList = true;
+                }
+                else if (err != CHIP_NO_ERROR)
+                {
+                    mDecodingFailed = true;
+                    break;
+                }
+            } while (0);
+        }
+        else
+        {
+            ++mAttributeStatusIBCount;
+        }
+
+        NextCallback().OnAttributeData(aPath, apData, aStatus);
+    }
+
+    void OnError(CHIP_ERROR aError) override { NextCallback().OnError(aError); }
+
+    void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
+    {
+        NextCallback().OnEventData(aEventHeader, apData, apStatus);
+    }
+
+    void OnDone(ReadClient * apReadClient) override { NextCallback().OnDone(apReadClient); }
+
+    void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override
+    {
+        NextCallback().OnSubscriptionEstablished(aSubscriptionId);
+    }
+
+    CHIP_ERROR OnResubscriptionNeeded(ReadClient * apReadClient, CHIP_ERROR aTerminationCause) override
+    {
+        return NextCallback().OnResubscriptionNeeded(apReadClient, aTerminationCause);
+    }
+
+    void OnDeallocatePaths(ReadPrepareParams && aReadPrepareParams) override
+    {
+        NextCallback().OnDeallocatePaths(std::move(aReadPrepareParams));
+    }
+
+    CHIP_ERROR OnUpdateDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder,
+                                             const Span<AttributePathParams> & aAttributePaths,
+                                             bool & aEncodedDataVersionList) override
+    {
+        return NextCallback().OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList);
+    }
+
+    CHIP_ERROR GetHighestReceivedEventNumber(Optional<EventNumber> & aEventNumber) override
+    {
+        return NextCallback().GetHighestReceivedEventNumber(aEventNumber);
+    }
+
+    void OnUnsolicitedMessageFromPublisher(ReadClient * apReadClient) override
+    {
+        NextCallback().OnUnsolicitedMessageFromPublisher(apReadClient);
+    }
+
+    void OnCASESessionEstablished(const SessionHandle & aSession, ReadPrepareParams & aSubscriptionParams) override
+    {
+        NextCallback().OnCASESessionEstablished(aSession, aSubscriptionParams);
+    }
+
+    BufferedReadCallback mBufferedCallback;
+    bool mSawEmptyList               = false;
+    bool mDecodingFailed             = false;
+    uint32_t mAttributeDataIBCount   = 0;
+    uint32_t mAttributeStatusIBCount = 0;
+};
+
 class TestReadCallback : public app::ReadClient::Callback
 {
 public:
@@ -138,10 +250,13 @@ class TestReadCallback : public app::ReadClient::Callback
 
     void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override { mOnSubscriptionEstablished = true; }
 
+    void OnError(CHIP_ERROR aError) override { mReadError = aError; }
+
     uint32_t mAttributeCount        = 0;
     bool mOnReportEnd               = false;
     bool mOnSubscriptionEstablished = false;
-    app::BufferedReadCallback mBufferedCallback;
+    CHIP_ERROR mReadError           = CHIP_NO_ERROR;
+    TestBufferedReadCallback mBufferedCallback;
 };
 
 void TestReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData,
@@ -276,7 +391,7 @@ CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, ap
     {
     case kTestListAttribute:
         return aEncoder.EncodeList([](const auto & encoder) {
-            for (int i = 0; i < 5; i++)
+            for (int i = 0; i < kListAttributeItems; i++)
             {
                 ReturnErrorOnFailure(encoder.Encode((uint8_t) gIterationCount));
             }
@@ -446,38 +561,74 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext)
     app::AttributePathParams attributePath(kTestEndpointId3, app::Clusters::UnitTesting::Id, kTestListAttribute);
     app::ReadPrepareParams readParams(sessionHandle);
 
-    readParams.mpAttributePathParamsList    = &attributePath;
-    readParams.mAttributePathParamsListSize = 1;
+    // Read the path twice, so we get two lists.  This make it easier to check
+    // for what happens when one of the lists starts near the end of a packet
+    // boundary.
+
+    AttributePathParams pathList[] = { attributePath, attributePath };
+
+    readParams.mpAttributePathParamsList    = pathList;
+    readParams.mAttributePathParamsListSize = ArraySize(pathList);
+
+    constexpr size_t maxPacketSize = kMaxSecureSduLengthBytes;
+    bool gotSuccessfulEncode       = false;
+    bool gotFailureResponse        = false;
 
     //
-    // We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2
-    // AttributeDataIBs into the packet. ~30-40 bytes covers a single AttributeDataIB, but let's 2-3x that
-    // to ensure we'll sweep from fitting 2 IBs to 3-4 IBs.
+    // Make sure we start off the packet size large enough that we can fit a
+    // single status response in it.  Verify that we get at least one status
+    // response.  Then sweep up over packet sizes until we're big enough to hold
+    // something like 7 IBs (at 30-40 bytes each, so call it 200 bytes) and check
+    // the behavior for all those cases.
     //
-    for (int i = 100; i > 0; i--)
+    for (uint32_t packetSize = 30; packetSize < 200; packetSize++)
     {
         TestReadCallback readCallback;
 
-        ChipLogDetail(DataManagement, "Running iteration %d\n", i);
+        ChipLogDetail(DataManagement, "Running iteration %d\n", packetSize);
 
-        gIterationCount = (uint32_t) i;
+        gIterationCount = packetSize;
 
-        app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(static_cast<uint32_t>(850 + i));
+        app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(
+            static_cast<uint32_t>(maxPacketSize - packetSize));
 
         app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback,
                                    app::ReadClient::InteractionType::Read);
 
         NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR);
 
         ctx.DrainAndServiceIO();
-        NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
 
-        //
-        // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in
-        // TestReadCallback::OnAttributeData
-        //
-        NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1);
-        readCallback.mAttributeCount = 0;
+        // Up until our packets are big enough, we might just keep getting
+        // errors due to the inability to encode even a single IB in a packet.
+        // But once we manage a successful encode, we should not have any more failures.
+        if (!gotSuccessfulEncode && readCallback.mReadError != CHIP_NO_ERROR)
+        {
+            gotFailureResponse = true;
+            // Check for the right error type.
+            NL_TEST_ASSERT(apSuite,
+                           StatusIB(readCallback.mReadError).mStatus == Protocols::InteractionModel::Status::ResourceExhausted);
+        }
+        else
+        {
+            gotSuccessfulEncode = true;
+
+            NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
+
+            //
+            // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in
+            // TestReadCallback::OnAttributeData.  The attribute count is 1
+            // because the buffered callback treats the second read's path as being
+            // just a replace of the first read's path and buffers it all up as a
+            // single value.
+            //
+            NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1);
+            readCallback.mAttributeCount = 0;
+
+            // Check that we never saw an empty-list data IB.
+            NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mDecodingFailed);
+            NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mSawEmptyList);
+        }
 
         NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
 
@@ -490,6 +641,9 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext)
         }
     }
 
+    // If this fails, our smallest packet size was not small enough.
+    NL_TEST_ASSERT(apSuite, gotFailureResponse);
+
     emberAfClearDynamicEndpoint(0);
 }