Add a crashy program for SetString test. #4443
Conversation
|
|
||
| TEST(ExtensionTest, SetString) { | ||
| typedef CobaltExtensionCrashHandlerApi ExtensionApi; | ||
| const char* kExtensionName = kCobaltExtensionCrashHandlerName; |
There was a problem hiding this comment.
Is this variable needed? It seems more clear to just pass kCobaltExtensionCrashHandlerName directly to SbSystemGetExtension().
| #include "starboard/system.h" | ||
| #include "testing/gtest/include/gtest/gtest.h" | ||
|
|
||
| const char* long_string = |
There was a problem hiding this comment.
Is it important that this string is so long? If so, I'd recommend adding a comment explaining why this length is an important thing to test.
There was a problem hiding this comment.
This time it is not critical. Initial idea was to test the maximum amount of data, transferred in annotations, but implementation should be different. Fixed.
| // https://github.com/google/googletest/blob/main/docs/advanced.md#death-tests, | ||
| // except, it doesn't spawn another process, but terminates itself. | ||
| // It is intended to be started from a host machine on a target device | ||
| // to generate a coredump, which is verified on the host. |
There was a problem hiding this comment.
This test doesn't actually do any verifications about the expected annotations in the core dump. Will those be added later? Or is the expectation that the core dump, and its annotations, will be verified manually after this test is run?
There was a problem hiding this comment.
Such happened, target part of the test is cross-platform. But verification code, apparently, can be platform-specific only. I've implemented it for PlayStation, but I see no way to implement similar for UWP/Windows. This is current status.
There was a problem hiding this comment.
I support getting the code in, if it helps bringing up integration tests.
There was a problem hiding this comment.
We could enable platforms to add platform-specific verification code, if we wanted to (and there's a viable way to automatically do the verification locally on the platform, for example by examining an expected crash dump stored on the file system).
One way to do that would be by adding a new method to the CrashHandler Starboard extension itself.
There was a problem hiding this comment.
I have had a doubt about possible way to make verification locally on the platform. This way is preferable, as doesn't involve host side. I can consider this way.
There was a problem hiding this comment.
@hlwarriner I see no a viable way to automatically do the verification locally on the platform.
b/358663298 Change-Id: If8931dac92837bc7295006e7c3e57fe8f91dc627
v-kryachko
force-pushed
the
25.lts.1+
branch
from
73819a1 to
c06d710
Compare
| @@ -29,3 +29,13 @@ target(gtest_target_type, "extension_test") { | |||
| deps += cobalt_platform_dependencies | |||
| } | |||
| } | |||
|
|
|||
| target(gtest_target_type, "setstring_test") { | |||
There was a problem hiding this comment.
A downside of adding this setstring_test is that it would need to be built and run manually by developers, and won't run automatically in any CI. A cursory look shows that InitAndRunAllTests() is unnecessary since it doesn't do anything special. Why not just add setstring_test.cc to a larger test-only build target, e.g. "extension_test" in l.15 ?
There was a problem hiding this comment.
We could alternatively just add this test target to cobalt/build/cobalt_configuration.py so that it's run in CI.
I think there's limited utility in running it automatically, though, as long as it requires manual steps in order to verify the result (see other comment thread).
There was a problem hiding this comment.
@yell0wd0g I can suggest rename the executable to 'crashy_process' or alike and remove use of gtest library to avoid confusion with tests, because it is not.
There was a problem hiding this comment.
Ah I didn't notice the need for manual test.
An interesting pattern to consider then would be the "MANUAL_FooTest" used by Chromium (some doc here, one example here). Test cases go into a larger binary (easier to deploy and conceptualize) but don't run by default unless the test binary is run with the "--run-manual" command line flag.
Perhaps it'd be overkill for this case since it's going to 25.lts, but certainly something to consider for 26+.
Otherwise I'm good with other reviewers' LGTMs. Thanks!
b/358663298
Change-Id: If8931dac92837bc7295006e7c3e57fe8f91dc627