Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for SetString in CrashHandler #4040

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add test for SetString in CrashHandler #4040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions starboard/extension/extension_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds. I agree with your point about tests specifically designed for the platform and reopened b/358663298 to discuss.

}

const ExtensionApi* second_extension_api =
Expand Down
1 change: 1 addition & 0 deletions starboard/linux/shared/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
'SbSocketAddressTypes/SbSocketGetInterfaceAddressTest.SunnyDaySourceNotLoopback/type_ipv6',
],
'player_filter_tests': [test_filter.FILTER_ALL],
'extension_test': ['ExtensionTest.CrashHandler'],
}

_FILTERED_TESTS = {
Expand Down
1 change: 1 addition & 0 deletions starboard/linux/x64x11/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
_FILTERED_TESTS = {
# TODO(b/321315344): Re-enable once the coverage build is fixed.
'nplb': ['PosixMemoryMapTest.CanChangeMemoryProtection',],
'extension_test': ['ExtensionTest.CrashHandler'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }

}


Expand Down
Loading