Repository navigation
Add a crashy program for SetString test. - #4443
Conversation
kaidokert
left a comment
kaidokert
left a comment
There was a problem hiding this comment.
lgtm - lets just describe the intent clearly in a comment above the test, too.
|
|
||
| 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.
One nit: please update "which is verified on the host" to "which is manually verified on the host" in the comment.
Aside from that, if it's not feasible to add automated verification on any platforms, including PS5, then I'm fine with this change landing as is. I'm assuming this change adds value because a) it's difficult / not feasible for developers to set crash annotations manually on some platforms, like PS5, but b) developers are able to manually verify annotations in crash reports on these platforms. But please let me know if that assumption is wrong.
There was a problem hiding this comment.
This change looks good to me, aside from my previous comment. I'm approving it but let's let @joeltine take a look as well.
There was a problem hiding this comment.
Sorry, I've read all the comments and I'm still not clear how this test is going to be used. When will it run, by whom, and how will verification/assertion happen (manually or not)?
There was a problem hiding this comment.
@hlwarriner Updated the comment. And yes, your assumption about manual verification of crash annotations is correct.
@joeltine This test is used to generate a coredump file (mostly on a target device) with annotations. The annotations are verified manually by a test engineer, using platform-dependent host-side tools.
There was a problem hiding this comment.
Thanks for confirming.
I left a high level question in b/358663298 - can you please take a look?
73819a1 to
c06d710
Compare
| } | ||
| } | ||
|
|
||
| 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!
There was a problem hiding this comment.
@yell0wd0g Changed test name to MANUAL_SetString.
I hope, Cobalt team think about adding a test like 'browser_tests' in Chromium.
b/358663298 Change-Id: If8931dac92837bc7295006e7c3e57fe8f91dc627
b/358663298
Change-Id: If8931dac92837bc7295006e7c3e57fe8f91dc627