Testing using pw_unit_test #29342
Closed
Testing using pw_unit_test #29342
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
|
|
github-actions
bot
added
transport
system
setup payload
platform
lib
inet
crypto
controller
ble
app
gn
shell
tests
support
core
labels
|
PR #29342: Size comparison from b516ff4 to 120cfb1 Increases (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, psoc6, telink)
Decreases (50 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, linux, mbed, psoc6, qpg, telink)
Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, psoc6, qpg, telink)
|
|
PR #29342: Size comparison from 82a3e3d to 4cf84bc Increases (7 builds for bl702, cc32xx, psoc6)
Decreases (6 builds for bl702, bl702l, cc32xx)
Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, psoc6, qpg, telink)
|
andy31415
reviewed
Sep 22, 2023
|
PR #29342: Size comparison from fdb296e to f034fb1 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
* Rename old chip_test_suite template to chip_test_suite_using_nltest It will be removed in the future, when it's no longer needed. * Make new chip_test_suite template, using pw_test instead of chip_test * Rename a few GN targets to be consistent with pigweed Make them end with `.lib` or `.run` instead of `_lib` or `_run`.
|
PR #29342: Size comparison from fdb296e to afd6371 Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq and
carol-apple
|
|
Actually, that just happens for all PRs in this repo. Once a PR is non-draft and passes restyled, pullapprove requests review from various groups, some of them largish, |
This was referenced Apr 17, 2024
This was referenced Apr 22, 2024
Closed
This was referenced Apr 23, 2024
This was referenced Apr 25, 2024
This was referenced Apr 25, 2024
This was referenced May 6, 2024
This was referenced May 17, 2024
This was referenced Jun 4, 2024
This was referenced Jun 12, 2024
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
Matter currently uses nlunit-test as a testing framework, which isn't maintained anymore. Moreover, the output it gives when some assertion fails is not very informative, eg. when asserting the value of some expression, it doesn't print what the expected and actual values were.
Changes
All unit tests have been rewritten to use the pw_unit_test framework instead. Since pigweed is already a dependency of Matter, and was developed with embedded devices in mind, it seemed a good choice. It also aims to be source-compatible with googletest, so most developers will feel right at home using it.
Build system
pw_unit_testadded to./.gn./build/chip/chip_test_suite.gnichanged, so it invokespw_testbuild/chip/chip_test.gniandbuild/chip/chip_test_group.gni, to keep target naming consistent**/BUILD.gnUnit test files
The overwhelming majority of changes were made semi-automatically using a simple regex-based rewriting script. This crude method worked quite well, though most files needed some small manual intervention afterwards. The actual testing logic was unchanged - the rewritten files ought to behave in exactly the same way as before. These are the kinds of changes made:
<gtest/gtest.h>instead of<nlunit-test.h>NL_TEST_ASSERT(suite, condition)withEXPECT_TRUE(condition)nlTestSuite * apSuiteparameter from functionsTEST,TEST_F, orSTATIC_TESTTestContexts into test fixture classes as static membersCHIP_REGISTER_TEST_SUITE,nlTest[],nlTestSuite, etc.Unlike
nlunit-test, testcases are run bypw_unit_testin the order in which they were declared in the source code. To keep the run-time order of testcases unchanged, sometimes their source-code declaration order had to be reshuffled (causing thousand-line-long diffs, even though nothing substantial changed!).Testing
Running
./scripts/tests/all_tests.shshows that all test suites compile and run successfully.Followups
The following problems were encountered, but couldn't be fixed "properly", because that would require changing the testing code (and risk breaking it). Instead, provisional workarounds were applied:
pw_unit_test. However, we also have the-Werrorflag, which causes warnings to get treated as errors. As a result existing code no longer compiles.-Wno-error=flags toBUILD.gnfilessrc/app/testsdepend on symbols defined in other test files, causing linker errors unless we were to build them as a single static library. But in pw_unit_test, testcases register themselves automatically via static initialization, so we can't link test suites together, unless we want all of them to run.src/app/tests/AppTestContext.cpp)These tasks were left to be dealt with in the future, in order not to introduce too many changes at once:
ASSERT_TRUE(abc == xyz);withASSERT_EQ(abc, xyz), etc.nlunit-testfrom the source tree