Add test for SetString in CrashHandler #4040
Conversation
MSoliankoLuxoft
force-pushed
the
crash_handler
branch
from
1481c6e
to
6277d7a
Compare
MSoliankoLuxoft
force-pushed
the
crash_handler
branch
4 times, most recently
from
c4a76d7
to
4420d44
Compare
b/358663298
MSoliankoLuxoft
force-pushed
the
crash_handler
branch
from
4420d44
to
9410542
Compare
niranjanyardi
requested review from
hlwarriner
and removed request for
hlwarriner and
niranjanyardi
| @@ -241,6 +241,7 @@ TEST(ExtensionTest, CrashHandler) { | |||
|
|
|||
| if (extension_api->version >= 2) { | |||
| EXPECT_NE(extension_api->SetString, nullptr); | |||
| EXPECT_TRUE(extension_api->SetString("test", "test")); | |||
There was a problem hiding this comment.
In my opinion this isn't giving us very much coverage of PS5's implementation of the Starboard Extension - for example, it would still pass if PS5's implementation of SetString() just returned true without doing any real work.
Do you have any other ideas for testing PS5's implementation?
There was a problem hiding this comment.
I completely agree with you. However, in these tests, we are only covering the Starboard API, and unfortunately, through the extension API, we only have access to SetString. "I think that in order to cover more than just SetString and to ensure that other functionality is also tested, we would likely need to write tests specifically designed for the platform, rather than general tests as we have in this case.
There was a problem hiding this comment.
Sounds. I agree with your point about tests specifically designed for the platform and reopened b/358663298 to discuss.
| @@ -19,6 +19,7 @@ | |||
| _FILTERED_TESTS = { | |||
| # TODO(b/321315344): Re-enable once the coverage build is fixed. | |||
| 'nplb': ['PosixMemoryMapTest.CanChangeMemoryProtection',], | |||
| 'extension_test': ['ExtensionTest.CrashHandler'], | |||
There was a problem hiding this comment.
I would prefer that we not disable this test case for all x64x11 platforms.
If we do move forward with a test like the one currently added in this PR, I think we could restrict this filter to just MODULAR builds. Another idea, which I actually prefer, is to revert the changes to starboard/shared/starboard/crash_handler.cc from https://github.com/youtube/cobalt/pull/1330/files and instead make a change so that MODULAR builds don't even implement the CrashHandler Starboard Extension.
There was a problem hiding this comment.
I think it would be better to use the test in the filter, but for _MODULAR_BUILD, similar to how it is done in cobalt/starboard/linux/shared/test_filters.py. If we revert this commit https://github.com/youtube/cobalt/pull/1330/files, when SetString is called and it uses InsertCrashpadAnnotation, we would need to call the function InstallCrashpadHandler(const std::string& ca_certificates_path) before usage, otherwise, an error will occur:
if (!SignalHandler::Get()) {
DLOG(ERROR) << "Crashpad isn't enabled";
return false;
}
b/358663298