From e545fdfa947bf37d8aad9a99a1d437b4c16b9dba Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 22 Apr 2022 11:57:56 +0800 Subject: [PATCH] Resolve comments --- src/app/tests/TestCommandInteraction.cpp | 1 - src/app/tests/TestWriteInteraction.cpp | 16 ---------------- src/messaging/ReliableMessageContext.h | 4 ++-- src/messaging/tests/MessagingContext.h | 2 +- src/transport/raw/tests/NetworkTestHelpers.h | 5 +++-- src/transport/tests/BUILD.gn | 7 +------ src/transport/tests/LoopbackTransportManager.h | 2 +- 7 files changed, 8 insertions(+), 29 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 4543a1441c017b..298896ac3d69ff 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -677,7 +677,6 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(n chip::isCommandDispatched = false; GenerateInvokeRequest(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/, messageIsTimed); err = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), transactionIsTimed); - ctx.DrainAndServiceIO(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(apSuite, chip::isCommandDispatched == (messageIsTimed == transactionIsTimed)); } diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 34fdd58a922496..6c286786990617 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -276,21 +276,6 @@ void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apCont TestContext & ctx = *static_cast(apContext); - // - // We have to enable async dispatch here to ensure that the exchange - // gets correctly closed out in the test below. Otherwise, the following happens: - // - // 1. WriteHandler generates a response upon OnWriteRequest being called. - // 2. Since there is no matching active client-side exchange for that request, the IM engine - // handles it incorrectly and treats it like an unsolicited message. - // 3. It is invalid to receive a WriteResponse as an unsolicited message so it correctly sends back - // a StatusResponse containing an error to that message. - // 4. Without unwinding the existing call stack, a response is received on the same exchange that the handler - // generated a WriteResponse on. This exchange should have been closed in a normal execution model, but in - // a synchronous model, the exchange is still open, and the status response is sent to the WriteHandler. - // 5. WriteHandler::OnMessageReceived is invoked, and it correctly asserts. - // - constexpr bool allBooleans[] = { true, false }; for (auto messageIsTimed : allBooleans) { @@ -327,7 +312,6 @@ void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apCont NL_TEST_ASSERT(apSuite, status == Status::UnsupportedAccess); } - ctx.DrainAndServiceIO(); ctx.DrainAndServiceIO(); Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index b702face453b94..d60c057f61086f 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -199,10 +199,10 @@ class ReliableMessageContext kFlagWillSendMessage = (1u << 8), /// When set, we have had Close() or Abort() called on us already. - kFlagClosed = (1u << 10), + kFlagClosed = (1u << 9), /// When set, signifies that the exchange is requesting Sleepy End Device fast-polling mode. - kFlagFastPollingMode = (1u << 11), + kFlagFastPollingMode = (1u << 10), }; BitFlags mFlags; // Internal state flags diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index 2ae58bbb2d8224..e7bad99dd533cb 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -158,7 +158,7 @@ class MessagingContext : public PlatformMemoryUser Optional mSessionBobToFriends; }; -// LoopbackMessagingContext enriches MessagingContext with a async loopback transport +// LoopbackMessagingContext enriches MessagingContext with an async loopback transport class LoopbackMessagingContext : public LoopbackTransportManager, public MessagingContext { public: diff --git a/src/transport/raw/tests/NetworkTestHelpers.h b/src/transport/raw/tests/NetworkTestHelpers.h index a7f15097506a45..0b633cc5b57d53 100644 --- a/src/transport/raw/tests/NetworkTestHelpers.h +++ b/src/transport/raw/tests/NetworkTestHelpers.h @@ -36,8 +36,6 @@ namespace Test { class IOContext { public: - IOContext() {} - /// Initialize the underlying layers and test suite pointer CHIP_ERROR Init(); @@ -65,6 +63,8 @@ class LoopbackTransportDelegate { public: virtual ~LoopbackTransportDelegate() {} + + // Called by the loopback transport when it drops a message due to a nonzero mNumMessagesToDrop. virtual void OnMessageDropped() {} }; @@ -74,6 +74,7 @@ class LoopbackTransport : public Transport::Base void InitLoopbackTransport(System::Layer * systemLayer) { mSystemLayer = systemLayer; } void ShutdownLoopbackTransport() { + // Packets are allocated from platform memory, we should release them before Platform::MemoryShutdown while (!mPendingMessageQueue.empty()) mPendingMessageQueue.pop(); } diff --git a/src/transport/tests/BUILD.gn b/src/transport/tests/BUILD.gn index 620c7b651091e2..ad14819c67145d 100644 --- a/src/transport/tests/BUILD.gn +++ b/src/transport/tests/BUILD.gn @@ -19,14 +19,9 @@ import("//build_overrides/nlunit_test.gni") import("${chip_root}/build/chip/chip_test_suite.gni") -static_library("helpers") { - output_name = "libTestTransportHelpers" - output_dir = "${root_out_dir}/lib" - +source_set("helpers") { sources = [ "LoopbackTransportManager.h" ] - cflags = [ "-Wconversion" ] - public_deps = [ "${chip_root}/src/transport:transport", "${chip_root}/src/transport/raw", diff --git a/src/transport/tests/LoopbackTransportManager.h b/src/transport/tests/LoopbackTransportManager.h index bf4ae6f67db053..df44e5d46f94a1 100644 --- a/src/transport/tests/LoopbackTransportManager.h +++ b/src/transport/tests/LoopbackTransportManager.h @@ -99,8 +99,8 @@ class LoopbackTransportManager } private: - TransportMgr mTransportManager; Test::IOContext mIOContext; + TransportMgr mTransportManager; }; } // namespace Test