Fix read interaction test cases dependencies #30548
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a copy-paste typo in few test cases where an event response flag was checked even though no event was generated. This has worked because all these test cases have shared event manager state and once event was generated by one test case other tests could see it. Add initialize/terminate functions which setup/destroy event manager for every test case individually, so in the future it will not be possible to share event manager state between test cases.
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
pullapprove
bot
requested review from
tima-q,
tobiasgraf,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
andy31415
approved these changes
Nov 17, 2023
|
PR #30548: Size comparison from 523c19f to 0c9bbce Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30548: Size comparison from 64a6594 to bd1b6bf Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30548: Size comparison from a439aab to aa5fe8d Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
mbknust
suggested changes
Nov 21, 2023
There was a problem hiding this comment.
There are still dependencies between test cases. To reproduce the issue, change order of NL_TEST_DEFs to match the test function declaration order above:
diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp
index 9b70ec2d71..64b3621c7e 100644
--- a/src/app/tests/TestReadInteraction.cpp
+++ b/src/app/tests/TestReadInteraction.cpp
@@ -4914,6 +4914,16 @@ void TestReadInteraction::TestSubscriptionReportWithDefunctSession(nlTestSuite *
namespace {
const nlTest sTests[] = {
+ NL_TEST_DEF("CheckReadClient", chip::app::TestReadInteraction::TestReadClient),
+ NL_TEST_DEF("TestReadUnexpectedSubscriptionId", chip::app::TestReadInteraction::TestReadUnexpectedSubscriptionId),
+ NL_TEST_DEF("CheckReadHandler", chip::app::TestReadInteraction::TestReadHandler),
+ NL_TEST_DEF("TestReadClientGenerateAttributePathList", chip::app::TestReadInteraction::TestReadClientGenerateAttributePathList),
+ NL_TEST_DEF("TestReadClientGenerateInvalidAttributePathList",
+ chip::app::TestReadInteraction::TestReadClientGenerateInvalidAttributePathList),
+ NL_TEST_DEF("TestReadClientInvalidReport", chip::app::TestReadInteraction::TestReadClientInvalidReport),
+ NL_TEST_DEF("TestReadHandlerInvalidAttributePath", chip::app::TestReadInteraction::TestReadHandlerInvalidAttributePath),
+ NL_TEST_DEF("TestReadClientGenerateOneEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateOneEventPaths),
+ NL_TEST_DEF("TestReadClientGenerateTwoEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateTwoEventPaths),
NL_TEST_DEF("TestReadRoundtrip", chip::app::TestReadInteraction::TestReadRoundtrip),
NL_TEST_DEF("TestReadRoundtripWithDataVersionFilter", chip::app::TestReadInteraction::TestReadRoundtripWithDataVersionFilter),
NL_TEST_DEF("TestReadRoundtripWithNoMatchPathDataVersionFilter",
@@ -4925,16 +4935,7 @@ const nlTest sTests[] = {
NL_TEST_DEF("TestReadWildcard", chip::app::TestReadInteraction::TestReadWildcard),
NL_TEST_DEF("TestReadChunking", chip::app::TestReadInteraction::TestReadChunking),
NL_TEST_DEF("TestSetDirtyBetweenChunks", chip::app::TestReadInteraction::TestSetDirtyBetweenChunks),
- NL_TEST_DEF("CheckReadClient", chip::app::TestReadInteraction::TestReadClient),
- NL_TEST_DEF("TestReadUnexpectedSubscriptionId", chip::app::TestReadInteraction::TestReadUnexpectedSubscriptionId),
- NL_TEST_DEF("CheckReadHandler", chip::app::TestReadInteraction::TestReadHandler),
- NL_TEST_DEF("TestReadClientGenerateAttributePathList", chip::app::TestReadInteraction::TestReadClientGenerateAttributePathList),
- NL_TEST_DEF("TestReadClientGenerateInvalidAttributePathList",
- chip::app::TestReadInteraction::TestReadClientGenerateInvalidAttributePathList),
- NL_TEST_DEF("TestReadClientGenerateOneEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateOneEventPaths),
- NL_TEST_DEF("TestReadClientGenerateTwoEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateTwoEventPaths),
- NL_TEST_DEF("TestReadClientInvalidReport", chip::app::TestReadInteraction::TestReadClientInvalidReport),
- NL_TEST_DEF("TestReadHandlerInvalidAttributePath", chip::app::TestReadInteraction::TestReadHandlerInvalidAttributePath),
+ NL_TEST_DEF("TestReadInvalidAttributePathRoundtrip", chip::app::TestReadInteraction::TestReadInvalidAttributePathRoundtrip),
NL_TEST_DEF("TestProcessSubscribeRequest", chip::app::TestReadInteraction::TestProcessSubscribeRequest),
/*
We need to figure out a way to run unit tests with an ICD build without affecting
@@ -4955,6 +4956,27 @@ const nlTest sTests[] = {
#endif // #if CHIP_CONFIG_ENABLE_ICD_SERVER
NL_TEST_DEF("TestSubscribeRoundtrip", chip::app::TestReadInteraction::TestSubscribeRoundtrip),
NL_TEST_DEF("TestSubscribeEarlyReport", chip::app::TestReadInteraction::TestSubscribeEarlyReport),
+ NL_TEST_DEF("TestSubscribeUrgentWildcardEvent", chip::app::TestReadInteraction::TestSubscribeUrgentWildcardEvent),
+ NL_TEST_DEF("TestSubscribeWildcard", chip::app::TestReadInteraction::TestSubscribeWildcard),
+ NL_TEST_DEF("TestSubscribePartialOverlap", chip::app::TestReadInteraction::TestSubscribePartialOverlap),
+ NL_TEST_DEF("TestSubscribeSetDirtyFullyOverlap", chip::app::TestReadInteraction::TestSubscribeSetDirtyFullyOverlap),
+ NL_TEST_DEF("TestSubscribeEarlyShutdown", chip::app::TestReadInteraction::TestSubscribeEarlyShutdown),
+ NL_TEST_DEF("TestSubscribeInvalidAttributePathRoundtrip",
+ chip::app::TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip),
+ NL_TEST_DEF("TestReadShutdown", chip::app::TestReadInteraction::TestReadShutdown),
+ NL_TEST_DEF("TestSubscribeInvalidInterval", chip::app::TestReadInteraction::TestSubscribeInvalidInterval),
+ NL_TEST_DEF("TestPostSubscribeRoundtripStatusReportTimeout",
+ chip::app::TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout),
+ NL_TEST_DEF("TestSubscribeRoundtripStatusReportTimeout",
+ chip::app::TestReadInteraction::TestSubscribeRoundtripStatusReportTimeout),
+ NL_TEST_DEF("TestReadChunkingStatusReportTimeout", chip::app::TestReadInteraction::TestReadChunkingStatusReportTimeout),
+ NL_TEST_DEF("TestReadReportFailure", chip::app::TestReadInteraction::TestReadReportFailure),
+ NL_TEST_DEF("TestSubscribeRoundtripChunkStatusReportTimeout",
+ chip::app::TestReadInteraction::TestSubscribeRoundtripChunkStatusReportTimeout),
+ NL_TEST_DEF("TestPostSubscribeRoundtripChunkStatusReportTimeout",
+ chip::app::TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout),
+ NL_TEST_DEF("TestPostSubscribeRoundtripChunkReportTimeout",
+ chip::app::TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout),
NL_TEST_DEF("TestPostSubscribeRoundtripChunkReport", chip::app::TestReadInteraction::TestPostSubscribeRoundtripChunkReport),
NL_TEST_DEF("TestReadClientReceiveInvalidMessage", chip::app::TestReadInteraction::TestReadClientReceiveInvalidMessage),
NL_TEST_DEF("TestSubscribeClientReceiveInvalidStatusResponse",
@@ -4970,37 +4992,15 @@ const nlTest sTests[] = {
NL_TEST_DEF("TestSubscribeClientReceiveUnsolicitedReportMessageWithInvalidSubscriptionId",
chip::app::TestReadInteraction::TestSubscribeClientReceiveUnsolicitedReportMessageWithInvalidSubscriptionId),
NL_TEST_DEF("TestReadChunkingInvalidSubscriptionId", chip::app::TestReadInteraction::TestReadChunkingInvalidSubscriptionId),
- NL_TEST_DEF("TestReadHandlerMalformedReadRequest1", chip::app::TestReadInteraction::TestReadHandlerMalformedReadRequest1),
- NL_TEST_DEF("TestReadHandlerMalformedReadRequest2", chip::app::TestReadInteraction::TestReadHandlerMalformedReadRequest2),
NL_TEST_DEF("TestReadHandlerMalformedSubscribeRequest",
chip::app::TestReadInteraction::TestReadHandlerMalformedSubscribeRequest),
+ NL_TEST_DEF("TestReadHandlerMalformedReadRequest1", chip::app::TestReadInteraction::TestReadHandlerMalformedReadRequest1),
+ NL_TEST_DEF("TestReadHandlerMalformedReadRequest2", chip::app::TestReadInteraction::TestReadHandlerMalformedReadRequest2),
NL_TEST_DEF("TestSubscribeSendUnknownMessage", chip::app::TestReadInteraction::TestSubscribeSendUnknownMessage),
NL_TEST_DEF("TestSubscribeSendInvalidStatusReport", chip::app::TestReadInteraction::TestSubscribeSendInvalidStatusReport),
NL_TEST_DEF("TestReadHandlerInvalidSubscribeRequest", chip::app::TestReadInteraction::TestReadHandlerInvalidSubscribeRequest),
NL_TEST_DEF("TestSubscribeInvalidateFabric", chip::app::TestReadInteraction::TestSubscribeInvalidateFabric),
NL_TEST_DEF("TestShutdownSubscription", chip::app::TestReadInteraction::TestShutdownSubscription),
- NL_TEST_DEF("TestSubscribeUrgentWildcardEvent", chip::app::TestReadInteraction::TestSubscribeUrgentWildcardEvent),
- NL_TEST_DEF("TestSubscribeWildcard", chip::app::TestReadInteraction::TestSubscribeWildcard),
- NL_TEST_DEF("TestSubscribePartialOverlap", chip::app::TestReadInteraction::TestSubscribePartialOverlap),
- NL_TEST_DEF("TestSubscribeSetDirtyFullyOverlap", chip::app::TestReadInteraction::TestSubscribeSetDirtyFullyOverlap),
- NL_TEST_DEF("TestSubscribeEarlyShutdown", chip::app::TestReadInteraction::TestSubscribeEarlyShutdown),
- NL_TEST_DEF("TestSubscribeInvalidAttributePathRoundtrip",
- chip::app::TestReadInteraction::TestSubscribeInvalidAttributePathRoundtrip),
- NL_TEST_DEF("TestReadInvalidAttributePathRoundtrip", chip::app::TestReadInteraction::TestReadInvalidAttributePathRoundtrip),
- NL_TEST_DEF("TestSubscribeInvalidInterval", chip::app::TestReadInteraction::TestSubscribeInvalidInterval),
- NL_TEST_DEF("TestSubscribeRoundtripStatusReportTimeout",
- chip::app::TestReadInteraction::TestSubscribeRoundtripStatusReportTimeout),
- NL_TEST_DEF("TestPostSubscribeRoundtripStatusReportTimeout",
- chip::app::TestReadInteraction::TestPostSubscribeRoundtripStatusReportTimeout),
- NL_TEST_DEF("TestReadChunkingStatusReportTimeout", chip::app::TestReadInteraction::TestReadChunkingStatusReportTimeout),
- NL_TEST_DEF("TestReadReportFailure", chip::app::TestReadInteraction::TestReadReportFailure),
- NL_TEST_DEF("TestSubscribeRoundtripChunkStatusReportTimeout",
- chip::app::TestReadInteraction::TestSubscribeRoundtripChunkStatusReportTimeout),
- NL_TEST_DEF("TestPostSubscribeRoundtripChunkStatusReportTimeout",
- chip::app::TestReadInteraction::TestPostSubscribeRoundtripChunkStatusReportTimeout),
- NL_TEST_DEF("TestPostSubscribeRoundtripChunkReportTimeout",
- chip::app::TestReadInteraction::TestPostSubscribeRoundtripChunkReportTimeout),
- NL_TEST_DEF("TestReadShutdown", chip::app::TestReadInteraction::TestReadShutdown),
NL_TEST_DEF("TestSubscriptionReportWithDefunctSession",
chip::app::TestReadInteraction::TestSubscriptionReportWithDefunctSession),
NL_TEST_SENTINEL(),
|
@mbknust Please retest with latest commit |
|
PR #30548: Size comparison from a439aab to c4741dd Increases above 0.2%:
Increases (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (2 builds for linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30548: Size comparison from a439aab to eeec9ed Increases above 0.2%:
Increases (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (2 builds for linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
mbknust
approved these changes
Nov 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
There was a copy-paste typo in few test cases where an event response flag was checked even though no event was generated. This has worked because all these test cases have shared event manager state and once event was generated by one test case other tests could see it.
Test cases reordering in PR #30470 should no longer be needed.
Changes
Added initialize/terminate functions which setup/destroy event manager for every test case individually, so in the future it will not be possible to share event manager state between test cases.
Testing
CI will verify.
Manually tested that one can run every single test cases individually, which was previously impossible.