8000 increasing coverage for src/protocols/bdx folder. by gd-mauri · Pull Request #39975 · project-chip/connectedhomeip · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

increasing coverage for src/protocols/bdx folder. #39975

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.

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gd-mauri
Copy link
Contributor

Testing

Increased unit tests coverage for #37247

Tests added for src/protocols/bdx

@gd-mauri gd-mauri self-assigned this Jul 11, 2025
Copy link
Contributor
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds new test files to increase test coverage for the src/protocols/bdx directory. Several tests in TestTransferFacilitator.cpp and TestTransferDiagnosticLog.cpp appear to be incomplete placeholders.

Comment on lines 26 to 29
TEST_F(TestTransferDiagnosticLog, TestTransferDiagnosticLog)
{

}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test case is empty. Implement test logic to verify the functionality of BdxTransferDiagnosticLog or remove the test if it's a placeholder 1.

Style Guide References

Footnotes

  1. Tests should assert conditions or be removed.

Comment on lines 172 to 171
TEST_F(TestTransferFacilitator, InitiatesTransfer)
{
TestInitiator initiator;

auto r = initiator.InitiateTransfer(&gSystemLayerAndClock, TransferRole::kSender,
TransferSession::TransferInitData(), System::Clock::Seconds16(60), System::Clock::Milliseconds32(500));

ASSERT_EQ(r, CHIP_NO_ERROR); // Placeholder for actual test logic
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test case only asserts that the function returns CHIP_NO_ERROR. A unit test should verify the side effects of the function under test. For InitiateTransfer, this includes checking that a timer has been started and that a SendInit or ReceiveInit message is ready to be sent 1.

Style Guide References

Footnotes

  1. Tests should assert conditions.

Comment on lines 221 to 238
10000
TEST_F(TestTransferFacilitator, PollsForOutput)
{
TestInitiator initiator;

auto r = initiator.InitiateTransfer(&gSystemLayerAndClock, TransferRole::kSender,
TransferSession::TransferInitData(), System::Clock::Seconds16(60), System::Clock::Milliseconds32(500));

ASSERT_EQ(r, CHIP_NO_ERROR); // Placeholder for actual test logic

// hook a callback to the output handler
bool outputHandled = false;
initiator.mTransferSessionOutputHandler = [&outputHandled](TransferSession::OutputEvent & event) {
outputHandled = true;
};

// hook a callback to the StartTimerHook
bool timerStarted = false;
gSystemLayerAndClock.mStartTimerHook = [&timerStarted](auto, auto, void *) {
timerStarted = true;
};

// Simulate a poll
initiator.PollForOutput();

// Check if the poll was handled correctly
ASSERT_TRUE(outputHandled);

// Check if the timer was started
ASSERT_TRUE(timerStarted);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The StartTimerHook is set up after InitiateTransfer is called, so it misses verifying the timer started by that function. Consider setting up the hook before InitiateTransfer to verify that it also starts a timer as expected 1.

Style Guide References

Footnotes

  1. Tests should assert conditions.

Comment on lines 252 to 270
TEST_F(TestTransferFacilitator, TriggeringImmediatePollCallsPollForOutput)
{
TestInitiator initiator;

gSystemLayerAndClock.SetMonotonic(0_ms);

auto r = initiator.InitiateTransfer(&gSystemLayerAndClock, TransferRole::kSender,
TransferSession::TransferInitData(), System::Clock::Seconds16(60), System::Clock::Milliseconds32(500));

ASSERT_EQ(r, CHIP_NO_ERROR); // Placeholder for actual test logic

// hook a callback to the output handler
bool outputHandled = false;
initiator.mTransferSessionOutputHandler = [&outputHandled](TransferSession::OutputEvent & event) {
outputHandled = true;
};

// hook a callback to the StartTimerHook
bool timerStarted = false;
gSystemLayerAndClock.mStartTimerHook = [&timerStarted](auto, auto, void *) {
timerStarted = true;
};

// Schedule an immediate poll
initiator.ScheduleImmediatePoll();

// Advancing clock 1_ms triggers the immediate poll disregarding the poll frequency timer
gSystemLayerAndClock.AdvanceMonotonic(1_ms);

// Check if the poll was handled correctly
ASSERT_TRUE(outputHandled);

// Check if the timer was started
ASSERT_TRUE(timerStarted);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The StartTimerHook is set after InitiateTransfer, so it doesn't test that InitiateTransfer starts a timer. It would be good to verify all timer-starting calls 1.

Style Guide References

Footnotes

  1. Tests should assert conditions.

Comment on lines 289 to 299
TEST_F(TestTransferFacilitator, ResponderPreparesForTransfer)
{
TestResponder responder;

gSystemLayerAndClock.SetMonotonic(0_ms);

auto r = responder.PrepareForTransfer(&gSystemLayerAndClock, TransferRole::kReceiver,
TransferControlFlags::kReceiverDrive, 1024, System::Clock::Seconds16(60), System::Clock::Milliseconds32(500));

EXPECT_EQ(r, CHIP_NO_ERROR); // Placeholder for actual test logic
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test for ResponderPreparesForTransfer only checks the return value. Enhance this test to verify the side effects, such as ensuring a timer is started to poll for incoming messages 1.

Style Guide References

Footnotes

  1. Tests should assert conditions.

@gd-mauri gd-mauri force-pushed the increase_unit_test_coverage_for_protocols_bdx branch from 17519df to 9da2a51 Compare July 11, 2025 19:06
Copy link
github-actions bot commented Jul 11, 2025

PR #39975: Size comparison from 48704fe to 9da2a51

Full report (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
platform target config section 48704fe 9da2a51 change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1102622 1102622 0 0.0
RAM 179010 179010 0 0.0
bl702 lighting-app bl702+eth FLASH 656030 656030 0 0.0
RAM 134961 134961 0 0.0
bl702+wifi FLASH 833212 833212 0 0.0
RAM 124517 124517 0 0.0
bl706+mfd+rpc+littlefs FLASH 1065330 1065330 0 0.0
RAM 117373 117373 0 0.0
bl702l contact-sensor-app bl702l+mfd+littlefs FLASH 894876 894876 0 0.0
RAM 105660 105660 0 0.0
lighting-app bl702l+mfd+littlefs FLASH 978594 978594 0 0.0
RAM 109852 109852 0 0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 763128 763128 0 0.0
RAM 103368 103368 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 774668 774668 0 0.0
RAM 108536 108536 0 0.0
pump-app LP_EM_CC1354P10_6 FLASH 721008 721008 0 0.0
RAM 96940 96940 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 705300 705300 0 0.0
RAM 97148 97148 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 548850 548850 0 0.0
RAM 205144 205144 0 0.0
lock CC3235SF_LAUNCHXL FLASH 581842 581842 0 0.0
RAM 205344 205344 0 0.0
efr32 lock-app BRD4187C FLASH 955016 955016 0 0.0
RAM 126564 126564 0 0.0
BRD4338a FLASH 749468 749460 -8 -0.0
RAM 251912 251912 0 0.0
window-app BRD4187C FLASH 1049576 1049576 0 0.0
RAM 122760 122760 0 0.0
esp32 all-clusters-app c3devkit DRAM 102272 102272 0 0.0
FLASH 1780616 1780616 0 0.0
IRAM 83862 83862 0 0.0
m5stack DRAM 121156 121156 0 0.0
FLASH 1747894 1747894 0 0.0
IRAM 117071 117071 0 0.0
linux air-purifier-app debug unknown 4856 4856 0 0.0
FLASH 2796646 2796646 0 0.0
RAM 117320 117320 0 0.0
all-clusters-app debug unknown 5672 5672 0 0.0
FLASH 6198206 6198206 0 0.0
RAM 531216 531216 0 0.0
all-clusters-minimal-app debug unknown 5536 5536 0 0.0
FLASH 5473562 5473562 0 0.0
RAM 228008 228008 0 0.0
bridge-app debug unknown 5568 5568 0 0.0
FLASH 4807802 4807802 0 0.0
RAM 207712 207712 0 0.0
camera-app debug unknown 8976 8976 0 0.0
FLASH 6945771 6945771 0 0.0
RAM 230168 230168 0 0.0
camera-controller debug unknown 9216 9216 0 0.0
FLASH 14386923 14386923 0 0.0
RAM 661528 661528 0 0.0
chip-tool debug unknown 6272 6272 0 0.0
FLASH 14743639 14743639 0 0.0
RAM 655232 655232 0 0.0
chip-tool-ipv6only arm64 unknown 40672 40672 0 0.0
FLASH 12718695 12718695 0 0.0
RAM 701480 701480 0 0.0
closure-app debug unknown 5536 5536 0 0.0
FLASH 4790656 4790656 0 0.0
RAM 200584 200584 0 0.0
fabric-admin debug unknown 5952 5952 0 0.0
FLASH 12804169 12804169 0 0.0
RAM 654264 654264 0 0.0
fabric-bridge-app debug unknown 4816 4816 0 0.0
FLASH 4593134 4593134 0 0.0
RAM 193424 193424 0 0.0
fabric-sync debug unknown 5056 5056 0 0.0
FLASH 5741245 5741245 0 0.0
RAM 491728 491728 0 0.0
lighting-app debug+rpc+ui unknown 6280 6280 0 0.0
FLASH 5694593 5694593 0 0.0
RAM 209944 209944 0 0.0
lock-app debug unknown 5488 5488 0 0.0
FLASH 4836482 4836482 0 0.0
RAM 197192 197192 0 0.0
ota-provider-app debug unknown 4856 4856 0 0.0
FLASH 4446986 4446986 0 0.0
RAM 186112 186112 0 0.0
ota-requestor-app debug unknown 4736 4736 0 0.0
FLASH 4519108 4519108 0 0.0
RAM 188984 188984 0 0.0
shell debug unknown 4288 4288 0 0.0
FLASH 3076572 3076572 0 0.0
RAM 147344 147344 0 0.0
thermostat-no-ble arm64 unknown 9832 9832 0 0.0
FLASH 4236319 4236319 0 0.0
RAM 233304 233304 0 0.0
tv-app debug unknown 5824 5824 0 0.0
FLASH 6106237 6106237 0 0.0
RAM 615976 615976 0 0.0
tv-casting-app debug unknown 5352 5352 0 0.0
FLASH 12888029 12888029 0 0.0
RAM 771728 771728 0 0.0
nxp contact mcxw71+release FLASH 624800 624800 0 0.0
RAM 63164 63164 0 0.0
lock mcxw71+release FLASH 776008 776008 0 0.0
RAM 67820 67820 0 0.0
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1632532 1632532 0 0.0
RAM 211104 211104 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1576708 1576708 0 0.0
RAM 208472 208472 0 0.0
light cy8ckit_062s2_43012 FLASH 1449500 1449500 0 0.0
RAM 197184 197184 0 0.0
lock cy8ckit_062s2_43012 FLASH 1481756 1481756 0 0.0
RAM 224904 224904 0 0.0
qpg lighting-app qpg6200+debug FLASH 744232 744232 0 0.0
RAM 94292 94292 0 0.0
lock-app qpg6200+debug FLASH 753852 753852 0 0.0
RAM 94320 94320 0 0.0
stm32 light STM32WB5MM-DK FLASH 465292 465292 0 0.0
RAM 141376 141376 0 0.0
telink bridge-app tl7218x FLASH 702340 702340 0 0.0
RAM 93600 93600 0 0.0
light-app-ota-compress-lzma-shell-factory-data tl3218x FLASH 794072 794072 0 0.0
RAM 44016 44016 0 0.0
light-app-ota-shell-factory-data tl7218x FLASH 782478 782478 0 0.0
RAM 100912 100912 0 0.0
light-switch-app-ota-compress-lzma-factory-data tl7218x_retention FLASH 709590 709590 0 0.0
RAM 54240 54240 0 0.0
light-switch-app-ota-compress-lzma-shell-factory-data tlsr9528a FLASH 746184 746184 0 0.0
RAM 77404 77404 0 0.0
light-switch-app-ota-factory-data tl3218x_retention FLASH 722910 722910 0 0.0
RAM 36996 36996 0 0.0
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 603014 603014 0 0.0
RAM 112532 112532 0 0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 818032 818036 4 0.0
RAM 99164 99164 0 0.0
tizen all-clusters-app arm unknown 5096 5096 0 0.0
FLASH 1695816 1695816 0 0.0
RAM 91444 91444 0 0.0
chip-tool-ubsan arm unknown 20768 20768 0 0.0
FLASH 21076194 21076194 0 0.0
RAM 9169420 9169420 0 0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant
0