Explicitly allow C++ in tests #66053
Comments
yperess
added
RFC
But pigweed was flat out rejected? #53760 |
A) it was rejected on the premises that we didn't have an in tree use. Their port of gTest was very useful to our downstream project and we want to upstream the tests for all the drivers. B) I'm not asking to add Pigweed. The issue is just saying "allow C++ in tests" |
|
Also, the latest discussion about Pigweed left it open to beeping added as an optional module in #53851. I haven't gotten around to cleaning up that proposal since we changed how modules are loaded |
|
Something I really dislike about this idea is it vastly limits devices that can run these tests. Sure native_sim and qemu would be fine, but we have micros with minute amounts of RAM, and some boards/compilers do not even support C++ mode out of the box |
|
@thedjnK is the alternative to just not write them? The way I'm looking at it is this. I have a lot of tests to write; using templates, classes, and lambdas drastically speeds things up and makes it easier to debug. I can either: A) write the tests in the chromium repo and they'll run to make sure that upstream changes don't break our projects Considering that the only tests for sensors in Zephyr were written by Google (AFAICT), I think it's safe to say that others either don't care (or use sensors) or don't have the headcount for it. I don't see why block the option of using C++. I'm not saying all tests should use it. Just that we allow it. |
|
If you want to test downstream in C++ then you can do as you like but you're trying to hold a proverbial gun to zephyr by demanding "use C++ for tests or we won't bother having any", without paying due care to what zephyr caters for
Unrelated to the topic really buy having a look, false. Looks like intel users did most of the contributions |
|
I see no reason at all not to explicitly allow tests in C++. Also, wondering if there is some more info on the gtest test harness. I think this is something that Meta might like to use as well. I started a zephyr shell utility that kind of mirrors gTest in #65437 specifically because someone had forked gtest so that it could be used in zephyr testsuites. |
Look, if that's how you want to take it I can't stop you. I'm just saying that this isn't the top priority for me. I'm going to write tests in a way that's fast and efficient. For me that's using C++ ideally using Pigweed + gTest. If Zephyr doesn't want those tests that's fine. I'm not making any threats nor do I mean to. These are the options I'm seeing, I can either put the code upstream and have it benefit everyone or downstream. Not a threat.
I'm not sure I'm seeing the same git history as you. There are currently 6 emulated sensors only in the Zephyr drivers tree. BMI160, F75303, ICM42688, ADLTC2990, AKM09918C, and AMD_SB_TSI. Google wrote the emulators for BMI160, F75303, ICM42688, AKM09918C, and AMD_SB_TSI It looks like a few emulators were written directly into the tests/ directory which makes them impossible to use in downstream testing, but those were for the ina230 and ina237 written by North River Systems Ltd. |
|
My concern with introducing an extra language in the code base (beyond the test/samples related to the language support itself) is potential maintainability implication, that is: difficulties for contributors to do changes and reviewer to review changes properly. It's no big deal for a case or two but imagine having the test/sample code base split in half between C and C++. The current expectation is that all contributors and reviewers can write/review some half decent C, I think by adding the language we are raising the bar and potentially lowering the quality considering that the current set of contributors are probably focused on good C practices - though I may be wrong about that, but that's my feeling anyway. |
|
I might be missing something, but doesn't Zephyr support C++ as a subsystem? Wouldn't having some tests written in C++ increase our testing of that subsystem as well and provide a better guarantee that all the functionality works? |
|
Yeah I think it makes sense to have C++ specific tests and samples if it's for increased coverage or has good demonstration value, what I think it's not ideal is mixing up languages based on the original author preference. For example, I think a sample/test showing how to properly use the sensor APIs in C++ is cool. One for testing basic framework functionalities that have nothing to do with the language, I think it should be in C. |
|
@yperess no problem with writing tests in any language as long as we end up with improved coverage and as long as this becomes part of the test framework of Zephyr, i.e. it is not some random person picking some other language based on preferences, but it is a documented and integrated way of writing tests |
|
I personally do not enjoy reviewing/reading C++ code as I find it hard to read and comprehend myself. If C++ code were allowed for test code it would probably naturally fall to the bottom of my personal review todo list consistently because of that. |
Not really, I'm writing the tests with ztest. There are some extra benefits that could be added via Pigweed, but that's not needed and is really a separate discussion. I wanted to add that bit of info in this issue just so people can see it. If we want to get into it. C++ gets us 90% of the way. gTest (via Pigweed or whatever other means) basically just gives us class inheritance for test fixtures. It's nice, but not the end of the world if we don't have. |
|
I'm happy to remove the reference to Pigweed from the issue, but I specifically had a very clear 1 line section for what I'm asking. Just that we OK the use of C++ in |
|
@yperess , one qeustion, how do we identy the testcases name form twister? will the c++ test report consist with our current twister C report? |
It could be up to the maintainer of the specific area if they prefer tests in C or C++ (and it would be good for maintainers to be clear / up front about their preference), but C should always be acceptable. |
Is it not the case here, in particular "some other language based on preferences"? |
I'm currently still using ztest. So nothing changes there. |
I agree that just "preference" is not enough. But C++ offers inheritance, lambdas, constexpr, and templates. All which are very useful tools when building generic tests. |
|
https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html
So if lambdas are C++11 then this would suggest they would not be allowed |
Actually, the default level of conformance is C++11. That implies, if someone chooses to use C++98 for their testsuite, they must not use features of C++ versions >= 98, which is more or less a given. The same would go for basically any revision of the C++ standard (or for that matter, the C standard). |
@yperess , I mean if the C++ cases is added, will this still be applicaible by current twister code scan? |
That is not how I read it, I read it as the code in the whole zephyr code base including base/core core, subsystem, module, test is all (the C++ parts) C++98 compliant, the default is C++11 most likely for out of tree users. Similar to how we specify cmake compatibility version 3.20 in cmake files, most people are probably using far newer versions of cmake. The discussion/issue that had that comment added is here: #55204 which seems to be a contentious thread but mentions that one of the supported zephyr compilers does not support anything newer than C++98, therefore it is my opinion that if C++ be allowed in tests (I'm neither for/against it) then this be the required rule, i.e. no C++11 features. Pinging @andyross as it was him that added the commit |
|
Architecture WG meeting 2023-12-05:
|
|
one more drawback of using c++ for test is the fact that we lack good samples and users look at tests for reference, if the reference code is in another language, that is is a problem. |
Not always, but I have often looked in the test directory for examples. Also, there are many hardware tests that a user would obviously like to run on his own hardware, but would probably need to install/bootstrap another compiler (compiler version). What happens if the maintainer + contributors leave the project due to personal reasons or company preferences change? (not that it has not happened so far) Different language is an additional hurdle to find a new maintainer/collaborator or to get a quick fix in time if CI fails. In general, the project tree should have fewer dependencies to be more fail-safe and robust. But maybe there can be optional tests as modules, where the tests are written in language like C++, Rust, or APL. |
Also, C++ compatibility with C obviously shines compared to other languages. On other other hand, C++ is not really a single language. Ask what "modern C++" is and everyone will have a slightly different and evolving answer. Most answers will include "Don't write C code in C++" which will be a challenge in a C project. Bjarne Stroustrup is proposing mechanically enforced "profiles" to finally draw some lines in the sand https://lwn.net/Articles/949269/ tl;dr: which particular subset(s) of C++ are allowed should be well specified and maintained by Zephyr and that's not just a version like C++98, it's more than that. Not sure whether it could be enforced and how.
|
yes, that is what I am saying :) |
Oooph. I'm really not the person to ask about policy decisions. That was just a practical patch, we had features being used that didn't work on xt-xcc and the kconfig that was supposed to guard them wasn't. But on policy, and again just arguing practically: the overwhelming majority of Zephyr platforms have current compilers with C++20 features. And test code is just test code. If a platform isn't appropriate to a particular subsystem (e.g. we don't bother with networking tests on audio DSPs) then I don't see why tests of that subsystem need to be bound by its requirements (i.e. sure, write a network framework test in C++ because it'll never have to be built with xcc). Let's not go nuts, but I don't see why we can't be flexible. That said, the converse point is that tests aimed at general functionality for all platforms need to be really conservative about language choice, and for the same reason (e.g. absolutely no C++ in tests/kernel/threads/thread_apis under any circumstances). |
This "upstream vs downstream" split sounds too binary / simplistic. The Linux kernel hates "out-of-tree" code, so you're either in or out. It tends to be quite binary like that. Many Linux people don't even care whether your out of tree code is open-source or not. But Zephyr is very different. Firstly, you could obviously open-source a Zephyr git branch with just your tests first; before this discussion converges. You said you're going to write these tests anyway. Unlike Linux, Zephyr makes an effort not to break APIs. So everyone could try your tests and there's a good chance they would just work or with minimal porting effort. Maybe even better: would it be technically possible to isolate these new tests in a separate git repo? At least initially. It would then be both 1) easier for users to download them with west and try (no git merge!), and: 2) easier for you to manage non-backwards compatible API changes. Tests which are popular, in active use and with a record of finding bugs would make a much more compelling C++ argument than test code that does not exist yet. They would also make discussions about C++ standards, variants and toolchains much better informed. |
It would be technically quite backwards to require all C++ code in Zephyr to only use C++98 features. What would even be the point them of supporting newer features? I think the better policy would be to maybe guarantee that a certain subset of the core C API (headers) must compile even with a C++98 compiler. But explicitly disallowing newer C++ features would be... a huge mistake. Imagine forcing everyone to have their own semi-functional workaround implementation of lambdas? That would really just weaken the project and wouldn't serve anyone. Plus, when there is an active Kconfig choice made, i.e. yes, I would like to enable this non-minimal feature or run this particular testsuite that requires these Kconfig options / C++ standard, there is an implicit contract made anyway. |
|
The way this conversation is drifting from "let's do C++" to "what C++ should we do" is interesting, and I'd imagine part of the reason some people prefer to dodge the subject entirely and prefer sticking with plain C. (or, some variation of plain C :-)) |
The more I think of this, the more important I find this argument. Zephyr tests and samples are often the only way for a user to see good examples of how to use a given Zephyr API. As most users tend to write Zephyr applications in C, it would be counter-intuitive to have the best API usage examples be in a different language. The other point - which was brought forward at the Arch WG meeting on 2023-12-05 - "Opening up for writing tests for any subsystem/driver class/... in C++ comes with the consequence of maintainers and other developers to maintain/debug/review C++ code, which not everybody may be comfortable with" is also very relevant to me. Zephyr is a C-based project, which allows users to write applications in C or C++, but the core of the project is C. I personally believe this to be one of the deciding factors for users choosing to use and contribute to Zephyr, and I would not want this to start sliding towards Zephyr being a multi-language project at its core. Opening up for writing tests in C++ for select subsystems could create confusion and likely discussions on why the maintainer of subsystem/driver class A accepts C++ test PRs, while the maintainer of subsystem/driver class B doesn't. It also raises the bar for whoever is to take over maintaining a giving area at a later point in time, if the previous maintainer accepted C++ tests, but the person interested in taking over doesn't feel comfortable with C++. My vote is no to accept C++ in Zephyr tests other than those explicitly testing C++ compatibility. |
|
One of the factors for tests is the ability for them to be maintainable (I don't think anyone is disagreeing with this). With the addition of the emulator backend API we're able to write a test once and ensure that all the drivers of the same class pass, here's a pseudo code example: TEST(Sample, TestReadingData) {
const struct device *dev = DEVICE_DT_GET(DT_NODELABEL(thing));
const struct emul *emulator = DEVICE_DT_GET(DT_NODELABEL(thing));
const uint8_t expected[] = {0, 1, 2, 3, 4};
special_buffer_backend_set_data(emulator, expected, ARRAY_SIZE(expected));
uint8_t result[ARRAY_SIZE(expected)];
ASSERT_OK(special_buffer_read(dev, result, ARRAY_SIZE(expected)));
ASSERT_MEM_EQ(expected, result);
}The above test works for a DT node TEST_P(Sample, TestReadingData) {
const struct device *dev = GetParam().first;
const struct emul *emulator = GetParam().second;
...
}
INSTANTIATE_TEST_SUITE_P(
MySampleInstance, Sample,
testing::Values(
std::make_pair(DEVICE_DT_GET(DT_NODELABEL(thing1)), DEVICE_DT_GET(DT_NODELABEL(thing1)),
std::make_pair(DEVICE_DT_GET(DT_NODELABEL(thing2)), DEVICE_DT_GET(DT_NODELABEL(thing2))
)
);Now the same test will run for Let's look at another aspect, constexpr int64_t Sum(std::vector<int> list) {
int64_t sum = 0;
for (auto &it : list) {
sum += it;
}
return sum;
}Now, your application doesn't even need to actually compile in I could keep going, but the final maintainability of a C++ test that's intended to be written once and run on N pairs of |
pure buffer testing is not a good example here, can we have an effective example for RTOS context and driver context, this is what we focus most.? |
|
closing as this was resolved by the TSC. |
While working on #69411, I discovered that (same |
For competeness. from the list (https://lists.zephyrproject.org/g/tsc/message/1870): |
Introduction
Writing good tests is difficult enough and making sure that there are good common utilities is paramount. With the addition of the
gtesttwister harness we've enabled the use of it in Twister. Pigweed (yes, I know, that word again) provides a port of gTest that can run in an embedded space. This can drastically and quickly improve our ability to test and is the current plan for our tests in downstream Chrome OS.The ask
I would like us to update https://docs.zephyrproject.org/latest/develop/languages/cpp/index.html#enabling-c-support to explicitly say that we can use C++ in the
tests/directory.Appendix
#65278
The text was updated successfully, but these errors were encountered: