Set of fixes to unit-tests code (#23071)

* [Fix] Execute TestLogBufferAsHex only if CHIP_PROGRESS_LOGGING

TestLogBufferAsHex test validates the LogBufferAsHex function
which uses ChipLogProgress().
If progress logging is disable this test failed.
Execute it only if CHIP_PROGRESS_LOGGING is defined.

Signed-off-by: ATmobica <[email protected]>

* [Fix] Fix PeerAddress string conversion test

Allow uppercase IPV6 string conversion. Check both cases.

Signed-off-by: ATmobica <[email protected]>

* [Fix] Fix ArgParser in support library

Fix parse args for embedded non posix long_opt implementation.
Disable a parse args unit test for non posix long_opt.
Fix duplicate id in option sets in ParseArgs unit test.

Signed-off-by: ATmobica <[email protected]>

* [Fix] Fix warnings of incorrect types in printf formatting

src/app/util/mock/attribute-storage.cpp
src/controller/tests/data_model/TestRead.cpp
src/lib/support/jsontlv/TlvJson.cpp

Signed-off-by: ATmobica <[email protected]>

* [Fix] Add missing assert header include in TestInetCommonPosix

Signed-off-by: ATmobica <[email protected]>

* [Fix] Fix TestReadInteraction implementation

Remove leftover debug printf.
Drain and service IO repeatedly until condition met.

Signed-off-by: ATmobica <[email protected]>

* [Fix] Fix controller unit tests

Change TestReadChunkingTests to TestEventChunkingTests in
TestEventChunkingTests.cpp

Signed-off-by: ATmobica <[email protected]>

Signed-off-by: ATmobica <[email protected]>

Author: ATmobica

src/app/tests/TestReadInteraction.cpp

@@ -1565,8 +1565,6 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a
         delegate.mGotEventResponse     = false;
         delegate.mNumAttributeResponse = 0;
 
-        printf("HereHere\n");
-
         err = engine->GetReportingEngine().SetDirty(dirtyPath1);
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
         err = engine->GetReportingEngine().SetDirty(dirtyPath2);
@@ -1918,13 +1916,16 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
             err = engine->GetReportingEngine().SetDirty(dirtyPath);
             NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-            ctx.DrainAndServiceIO();
-
             //
-            // Not sure why I had to add this, and didn't have cycles to figure out why.
-            // Tracked in Issue #17528.
+            // We need to DrainAndServiceIO() until attribute callback will be called.
+            // This is not correct behavior and is tracked in Issue #17528.
             //
-            ctx.DrainAndServiceIO();
+            int last;
+            do
+            {
+                last = delegate.mNumAttributeResponse;
+                ctx.DrainAndServiceIO();
+            } while (last != delegate.mNumAttributeResponse);
 
             NL_TEST_ASSERT(apSuite, delegate.mGotReport);
             // Mock endpoint3 has 13 attributes in total, and we subscribed twice.

src/controller/tests/TestEventChunking.cpp

@@ -542,10 +542,10 @@ nlTestSuite sSuite =
 
 } // namespace
 
-int TestReadChunkingTests()
+int TestEventChunkingTests()
 {
     gSuite = &sSuite;
     return chip::ExecuteTestsWithContext<TestContext>(&sSuite);
 }
 
-CHIP_REGISTER_TEST_SUITE(TestReadChunkingTests)
+CHIP_REGISTER_TEST_SUITE(TestEventChunkingTests)

src/controller/tests/data_model/TestRead.cpp

@@ -2643,8 +2643,8 @@ void TestReadInteraction::TestReadHandler_MultipleSubscriptionsWithDataVersionFi
             numSuccessCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1);
     });
 
-    ChipLogError(Zcl, "Success call cnt: %u (expect %u) subscription cnt: %u (expect %u)", numSuccessCalls,
-                 uint32_t(app::InteractionModelEngine::kReadHandlerPoolSize + 1), numSubscriptionEstablishedCalls,
+    ChipLogError(Zcl, "Success call cnt: %" PRIu32 " (expect %" PRIu32 ") subscription cnt: %" PRIu32 " (expect %" PRIu32 ")",
+                 numSuccessCalls, uint32_t(app::InteractionModelEngine::kReadHandlerPoolSize + 1), numSubscriptionEstablishedCalls,
                  uint32_t(app::InteractionModelEngine::kReadHandlerPoolSize + 1));
 
     NL_TEST_ASSERT(apSuite, numSuccessCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1));

src/inet/tests/TestInetCommonPosix.cpp

@@ -37,6 +37,7 @@
 #include "TestInetCommon.h"
 #include "TestInetCommonOptions.h"
 
+#include <assert.h>
 #include <errno.h>
 #include <vector>
 

src/lib/support/CHIPArgParser.cpp

@@ -292,6 +292,11 @@ bool ParseArgs(const char * progName, int argc, char * const argv[], OptionSet *
     OptionSet * curOptSet;
     OptionDef * curOpt;
     bool handlerRes;
+#if CHIP_CONFIG_NON_POSIX_LONG_OPT
+    int lastOptIndex    = 0;
+    int subOptIndex     = 0;
+    int currentOptIndex = 0;
+#endif // CHIP_CONFIG_NON_POSIX_LONG_OPT
 
     // The getopt() functions do not support recursion, so exit immediately with an
     // error if called recursively.
@@ -345,7 +350,36 @@ bool ParseArgs(const char * progName, int argc, char * const argv[], OptionSet *
         // Attempt to match the current option argument (argv[optind]) against the defined long and short options.
         optarg = nullptr;
         optopt = 0;
-        id     = getopt_long(argc, argv, shortOpts, longOpts, &optIndex);
+#if CHIP_CONFIG_NON_POSIX_LONG_OPT
+        // to check if index has changed
+        lastOptIndex = currentOptIndex;
+        // optind will not increment on error, this is why we need to keep track of the current option
+        // this is for use when getopt_long fails to find the option and we need to print the error
+        currentOptIndex = optind;
+        // if it's the first run, optind is not set and we need to find the first option ourselves
+        if (!currentOptIndex)
+        {
+            while (currentOptIndex < argc)
+            {
+                currentOptIndex++;
+                if (*argv[currentOptIndex] == '-')
+                {
+                    break;
+                }
+            }
+        }
+        // similarly we need to keep track of short opts index for groups like "-fba"
+        // if the index has not changed that means we are still analysing the same group
+        if (lastOptIndex != currentOptIndex)
+        {
+            subOptIndex = 0;
+        }
+        else
+        {
+            subOptIndex++;
+        }
+#endif // CHIP_CONFIG_NON_POSIX_LONG_OPT
+        id = getopt_long(argc, argv, shortOpts, longOpts, &optIndex);
 
         // Stop if there are no more options.
         if (id == -1)
@@ -356,10 +390,35 @@ bool ParseArgs(const char * progName, int argc, char * const argv[], OptionSet *
         {
             if (ignoreUnknown)
                 continue;
+#if CHIP_CONFIG_NON_POSIX_LONG_OPT
+            // getopt_long doesn't tell us if the option which failed to match is long or short so check
+            bool isLongOption = false;
+            if (strlen(argv[currentOptIndex]) > 2 && argv[currentOptIndex][1] == '-')
+            {
+                isLongOption = true;
+            }
+            if (optopt == 0 || isLongOption)
+            {
+                // getopt_long function incorrectly treats unknown long option as short opt group
+                if (subOptIndex == 0)
+                {
+                    PrintArgError("%s: Unknown option: %s\n", progName, argv[currentOptIndex]);
+                }
+            }
+            else if (optopt == '?')
+            {
+                PrintArgError("%s: Unknown option: -%c\n", progName, argv[currentOptIndex][subOptIndex + 1]);
+            }
+            else
+            {
+                PrintArgError("%s: Unknown option: -%c\n", progName, optopt);
+            }
+#else
             if (optopt != 0)
                 PrintArgError("%s: Unknown option: -%c\n", progName, optopt);
             else
                 PrintArgError("%s: Unknown option: %s\n", progName, argv[optind - 1]);
+#endif // CHIP_CONFIG_NON_POSIX_LONG_OPT
             goto done;
         }
 
@@ -369,7 +428,11 @@ bool ParseArgs(const char * progName, int argc, char * const argv[], OptionSet *
         {
             // NOTE: with the way getopt_long() works, it is impossible to tell whether the option that
             // was missing an argument was a long option or a short option.
+#if CHIP_CONFIG_NON_POSIX_LONG_OPT
+            PrintArgError("%s: Missing argument for %s option\n", progName, argv[currentOptIndex]);
+#else
             PrintArgError("%s: Missing argument for %s option\n", progName, argv[optind - 1]);
+#endif // CHIP_CONFIG_NON_POSIX_LONG_OPT
             goto done;
         }
 

src/lib/support/jsontlv/TlvJson.cpp

@@ -95,7 +95,7 @@ void InsertKeyValue(Json::Value & json, const KeyContext & keyContext, T val)
     }
     else if (keyContext.keyType == KeyContext::kStructField)
     {
-        snprintf(keyBuf, sizeof(keyBuf), "%" PRIu32, keyContext.key);
+        snprintf(keyBuf, sizeof(keyBuf), "%u", keyContext.key);
         json[keyBuf] = val;
     }
     else

src/lib/support/tests/TestBytesToHex.cpp

@@ -433,8 +433,10 @@ const nlTest sTests[] = {
     NL_TEST_DEF("TestBytesToHexErrors", TestBytesToHexErrors),                       //
     NL_TEST_DEF("TestBytesToHexUint64", TestBytesToHexUint64),                       //
     NL_TEST_DEF("TestHexToBytesAndUint", TestHexToBytesAndUint),                     //
-    NL_TEST_DEF("TestLogBufferAsHex", TestLogBufferAsHex),                           //
-    NL_TEST_SENTINEL()                                                               //
+#ifdef CHIP_PROGRESS_LOGGING
+    NL_TEST_DEF("TestLogBufferAsHex", TestLogBufferAsHex), //
+#endif
+    NL_TEST_SENTINEL() //
 };
 
 } // namespace

src/lib/support/tests/TestCHIPArgParser.cpp

@@ -137,7 +137,7 @@ static size_t sCallbackRecordCount = 0;
 static OptionDef sOptionSetA_Defs[] =
 {
     { "foo",  kNoArgument, '1'  },
-    { "bar",  kNoArgument, 1001 },
+    { "bar",  kNoArgument, 1002 },
     { "baz",  kArgumentRequired, 'Z'  },
     { }
 };
@@ -350,7 +350,7 @@ static void SimpleParseTest_VariousShortAndLongWithArgs()
     VerifyHandleOptionCallback(0, __FUNCTION__, &sOptionSetA, '1', "--foo", nullptr);
     VerifyHandleOptionCallback(1, __FUNCTION__, &sOptionSetB, 1000, "--run", "run-value");
     VerifyHandleOptionCallback(2, __FUNCTION__, &sOptionSetB, 's', "-s", nullptr);
-    VerifyHandleOptionCallback(3, __FUNCTION__, &sOptionSetA, 1001, "--bar", nullptr);
+    VerifyHandleOptionCallback(3, __FUNCTION__, &sOptionSetA, 1002, "--bar", nullptr);
     VerifyHandleOptionCallback(4, __FUNCTION__, &sOptionSetA, '1', "-1", nullptr);
     VerifyHandleOptionCallback(5, __FUNCTION__, &sOptionSetA, 'Z', "-Z", "baz-value");
     VerifyHandleOptionCallback(6, __FUNCTION__, &sOptionSetB, 1000, "--run", "run-value-2");
@@ -779,7 +779,12 @@ int TestCHIPArgParser(void)
     UnknownOptionTest_UnknownShortOptionBeforeKnown();
     UnknownOptionTest_UnknownLongOptionAfterArgs();
     UnknownOptionTest_IgnoreUnknownShortOption();
+
+    /* Skip this test because the parser successfully captures all the options
+       but the error reporting is incorrect in this case due to long_opt limitations */
+#ifndef CHIP_CONFIG_NON_POSIX_LONG_OPT
     UnknownOptionTest_IgnoreUnknownLongOption();
+#endif // !CHIP_CONFIG_NON_POSIX_LONG_OPT
 
     MissingValueTest_MissingShortOptionValue();
     MissingValueTest_MissingLongOptionValue();

src/transport/raw/tests/TestPeerAddress.cpp

@@ -93,8 +93,11 @@ void TestToString(nlTestSuite * inSuite, void * inContext)
     {
         IPAddress::FromString("1223::3456:789a", ip);
         PeerAddress::UDP(ip, 8080).ToString(buff);
+        // IPV6 does not specify case
+        int res1 = strcmp(buff, "UDP:[1223::3456:789a]:8080");
+        int res2 = strcmp(buff, "UDP:[1223::3456:789A]:8080");
 
-        NL_TEST_ASSERT(inSuite, !strcmp(buff, "UDP:[1223::3456:789a]:8080"));
+        NL_TEST_ASSERT(inSuite, (!res1 || !res2));
     }
 
     {