Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReadHandler] Removal of test flags #28421

Merged
merged 6 commits into from
Aug 3, 2023
Merged

[ReadHandler] Removal of test flags #28421

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d00fdce
Removed reportScheduler test flags and made TestReadInteractin.cpp wa…
lpbeliveau-silabs Jul 26, 2023
c7fb1d1
Restyled by clang-format
restyled-commits Jul 31, 2023
1f56ddb
Added a driveAndServiceIO after waiting for max in test where we shou…
lpbeliveau-silabs Aug 1, 2023
146cc01
Moved ctx.GetIOContext().DriveIO(); after each tests to garantee this…
lpbeliveau-silabs Aug 1, 2023
5c605c3
Implemented a mock clock in TestReadInteraction to reduce wait loops
lpbeliveau-silabs Aug 2, 2023
e7fe784
Removed more loops and added comment on loop left in the code
lpbeliveau-silabs Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 57 additions & 119 deletions src/app/tests/TestReadInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,17 @@ chip::EndpointId kInvalidTestEndpointId = 3;
chip::DataVersion kTestDataVersion1 = 3;
chip::DataVersion kTestDataVersion2 = 5;

static chip::System::Clock::Internal::MockClock mockClock;
static chip::System::Clock::ClockBase * realClock;
lpbeliveau-silabs marked this conversation as resolved.
Show resolved Hide resolved

class TestContext : public chip::Test::AppContext
{
public:
static int Initialize(void * context)
{
realClock = &chip::System::SystemClock();
chip::System::Clock::Internal::SetSystemClockForTesting(&mockClock);

if (AppContext::Initialize(context) != SUCCESS)
return FAILURE;

Expand All @@ -97,6 +103,7 @@ class TestContext : public chip::Test::AppContext
static int Finalize(void * context)
{
chip::app::EventManagement::DestroyEventManagement();
chip::System::Clock::Internal::SetSystemClockForTesting(realClock);

if (AppContext::Finalize(context) != SUCCESS)
return FAILURE;
Expand Down Expand Up @@ -1598,17 +1605,9 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a

// Test report with 2 different path

// Wait for min interval to elapse
System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds))
{
break;
}
ctx.GetIOContext().DriveIO();
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds));
ctx.GetIOContext().DriveIO();

delegate.mGotReport = false;
delegate.mGotEventResponse = false;
Expand All @@ -1626,17 +1625,10 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);

// Test report with 2 different path, and 1 same path
// Wait for min interval to elapse
startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds))
{
break;
}
ctx.GetIOContext().DriveIO();
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds));
ctx.GetIOContext().DriveIO();

delegate.mGotReport = false;
delegate.mNumAttributeResponse = 0;
err = engine->GetReportingEngine().SetDirty(dirtyPath1);
Expand All @@ -1652,17 +1644,10 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);

// Test report with 3 different path, and one path is overlapped with another
// Wait for min interval to elapse
startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds))
{
break;
}
ctx.GetIOContext().DriveIO();
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds));
ctx.GetIOContext().DriveIO();

delegate.mGotReport = false;
delegate.mNumAttributeResponse = 0;
err = engine->GetReportingEngine().SetDirty(dirtyPath1);
Expand All @@ -1678,17 +1663,10 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);

// Test report with 3 different path, all are not overlapped, one path is not interested for current subscription
// Wait for min interval to elapse
startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds))
{
break;
}
ctx.GetIOContext().DriveIO();
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMinIntervalFloorSeconds));
ctx.GetIOContext().DriveIO();

delegate.mGotReport = false;
delegate.mNumAttributeResponse = 0;
err = engine->GetReportingEngine().SetDirty(dirtyPath1);
Expand All @@ -1704,16 +1682,8 @@ void TestReadInteraction::TestSubscribeRoundtrip(nlTestSuite * apSuite, void * a
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);

// Test empty report
// Wait for max interval to elapse
startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds))
{
break;
}
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds));
ctx.GetIOContext().DriveIO();

NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled());
Expand Down Expand Up @@ -1788,8 +1758,7 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
System::Clock::Timestamp startTime = mockClock.GetMonotonicTimestamp();

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 2);
NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr);
Expand All @@ -1813,60 +1782,60 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite
nonUrgentDelegate.mGotEventResponse = false;
nonUrgentDelegate.mGotReport = false;

// wait for min interval 2 seconds (in test, we use 1.6 seconds considering the time variation), expect no event is
// wait for min interval 1 seconds (in test, we use 0.6 seconds considering the time variation), expect no event is
// received, then wait for 0.8 seconds, then the urgent event would be sent out
// currently DriveIOUntil will call `DriveIO` at least once, which means that if there is any CPU scheduling issues,
// there's a chance 1.9s will already have elapsed by the time we get there, which will result in DriveIO being called when
// it shouldn't. Better fix could happen inside DriveIOUntil, not sure the sideeffect there.

// Advance monotonic looping to allow events to trigger
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(600))
ctx.GetIOContext().DriveIO();
if ((mockClock.GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(600))
{
break;
}
ctx.GetIOContext().DriveIO(); // at least one IO loop is guaranteed
mockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the mock clock, why is spinning still needed? What's the difference between advancing by 10ms, and advancing once in one go? What exactly of the implementation is causing you to have to do that? Is the IO you expected not occurring?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's necessary here is the multiple DriveIO, I just realized it is indeed not essential to do the first loop. The second one however is only there in a sort of way to ensure enough async events are process and that they are processed in the right time.

In the "test subscribe urgent wildcard event", 3 driveIOs are needed: Timer expired, Engine Run, EventManagement (report gets received by the readclient).

For the other tests, it's also a mater of ensuring the multiple event for chunked report are handled before the timer expires so that the timer gets properly reschudeled at the right time.

I put the multiple Drive IOs on the first occurence and kept the loop on the second with some explanation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual approach is to have a DriveIOUntilIdle(). Is there a way to implement this with existing DriveIO ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a comment about this from the previous designer about using DriveIOUntil, whic explained why he had to rely on loops in the first place as DriveIOUntil would be suceptible to scheduling issues.

In the meantime, it relies on the SystemClock to timeout, which won't work due to usage of a mock clock.

I can confirm I had the same problems when I tried to use it.

We also have DrainAndServiceIO but there again the function description states:

  • ... This should run to completion
    • in well-behaved logic (i.e there isn't an indefinite ping-pong of messages transmitted back
    • and forth).

And there again I can confirm that in our case it doesn't work as we do have back and forth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpbeliveau-silabs back-and-forth should be OK for DrainAndServiceIO as long as it completes..... What actually fails if you use DrainAndServiceIO?

}
ctx.GetIOContext().DriveIO();

NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);
NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mGotEventResponse);

startTime = System::SystemClock().GetMonotonicTimestamp();
// Advance monotonic timestamp for min interval to elapse
startTime = mockClock.GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(800))
ctx.GetIOContext().DriveIO();
if ((mockClock.GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(800))
{
break;
}
ctx.GetIOContext().DriveIO(); // at least one IO loop is guaranteed
mockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
}
ctx.GetIOContext().DriveIO();

NL_TEST_ASSERT(apSuite, delegate.mGotEventResponse);
NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mGotEventResponse);

// Since we just sent a report for our urgent subscription, the min interval of the urgent subcription should have been
// updated
NL_TEST_ASSERT(apSuite,
reportScheduler->GetMinTimestampForHandler(delegate.mpReadHandler) >
System::SystemClock().GetMonotonicTimestamp());
reportScheduler->GetMinTimestampForHandler(delegate.mpReadHandler) > mockClock.GetMonotonicTimestamp());
NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty());
delegate.mGotEventResponse = false;

// For our non-urgent subscription, we did not send anything, so the min interval should of the non urgent subcription
// should be in the past
NL_TEST_ASSERT(apSuite,
reportScheduler->GetMinTimestampForHandler(nonUrgentDelegate.mpReadHandler) <
System::SystemClock().GetMonotonicTimestamp());
mockClock.GetMonotonicTimestamp());
NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty());

// Wait for the min interval timer to fire.
startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(2100))
{
break;
}
ctx.GetIOContext().DriveIO(); // at least one IO loop is guaranteed
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Milliseconds32(2100));
ctx.GetIOContext().DriveIO();

// No reporting should have happened.
NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);
Expand Down Expand Up @@ -2338,16 +2307,8 @@ void TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip(nlTestSuite
NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr);
delegate.mpReadHandler = engine->ActiveHandlerAt(0);

// Wait for max interval to elapse
System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds))
{
break;
}
}
// Advance monotonic timestamp for min interval to elapse
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds));
ctx.GetIOContext().DriveIO();

NL_TEST_ASSERT(apSuite, engine->GetReportingEngine().IsRunScheduled());
Expand Down Expand Up @@ -2540,15 +2501,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout(nlTestSu
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 2);

// Wait for max interval to elapse
System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds))
{
break;
}
}
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds));
ctx.GetIOContext().DriveIO();

delegate.mGotReport = false;
Expand Down Expand Up @@ -2898,15 +2851,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout(nlT
dirtyPath1.mEndpointId = Test::kMockEndpoint3;
dirtyPath1.mAttributeId = Test::MockAttributeId(4);

System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds))
{
break;
}
}
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds));
ctx.GetIOContext().DriveIO();

err = engine->GetReportingEngine().SetDirty(dirtyPath1);
Expand Down Expand Up @@ -3010,15 +2955,7 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout(nlTestSui
dirtyPath1.mEndpointId = Test::kMockEndpoint3;
dirtyPath1.mAttributeId = Test::MockAttributeId(4);

System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >=
System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds))
{
break;
}
}
mockClock.AdvanceMonotonic(System::Clock::Seconds16(readPrepareParams.mMaxIntervalCeilingSeconds));
ctx.GetIOContext().DriveIO();

err = engine->GetReportingEngine().SetDirty(dirtyPath1);
Expand Down Expand Up @@ -3127,26 +3064,27 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReport(nlTestSuite * ap

// wait for min interval 1 seconds(in test, we use 0.9second considering the time variation), expect no event is received,
// then wait for 0.5 seconds, then all chunked dirty reports are sent out, which would not honor minInterval
System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();
System::Clock::Timestamp startTime = mockClock.GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(900))
ctx.GetIOContext().DriveIO();
if ((mockClock.GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(900))
{
break;
}
ctx.GetIOContext().DriveIO();
mockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
}
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0);
startTime = System::SystemClock().GetMonotonicTimestamp();
startTime = mockClock.GetMonotonicTimestamp();
while (true)
{
if ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(500))
ctx.GetIOContext().DriveIO();
if ((mockClock.GetMonotonicTimestamp() - startTime) >= System::Clock::Milliseconds32(500))
{
break;
}
ctx.GetIOContext().DriveIO();
mockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
}
ctx.DrainAndServiceIO();
}
// Two chunked reports carry 4 attributeDataIB: 1 with a list of 3 items,
// and then one per remaining item.
Expand Down