Skip to content

cobalt/shell: Add Content Browser Tests #6430

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.

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Awallky
Copy link
Contributor

@Awallky Awallky commented Jul 15, 2025

This adds the minimum framework for content browser tests to be run on cobalt's content_shell_lib and content_shell_app. This adds only the content_main_runner_impl_browsertest and navigation_browsertest test suites, with most of the test sources from content/shell:content_browsertests omitted to reduce this PR's complexity.
It is a direct copy of the libraries under //content/test needed for content_browsertests.

Note: Overall, this should not affect the behavior or size of libcobalt or cobalt/shell. All of the copies are for testing and so that there are no dependencies upon content/shell.

Below is a detailed description and reasoning behind each change:

  • //BUILD.gn: Enables targets under cobalt/testing to be recognized and built
  • //build/android/pylib/gtest/gtest_test_instance.py - Needed for the cobalt tests to be recognized and properly run by the test runners.
  • //cobalt/shell/BUILD.gn:
    * Added a new android_shell_descriptors source set.
    * Made content_shell_lib_deps and content_shell_app_deps visible to //cobalt/testing/*.
    * Added a pak target to repack resources.
    • //cobalt/shell/android/BUILD.gn - Enables the cobat/shell to be built and packaged into a test library for use by the test runners
    • //cobalt/testing/browser_tests/BUILD.gn: This is a new file that defines the cobalt_browsertests target and its dependencies.
    • //content/public/test/android/javatests/src/org/chromium/content_public/browser/test/mock/MockWebContents.java: Added onResume and onFreeze methods, since builds failed without them.
    • //cobalt/shell/android/browsertests/src/org/chromium/content_shell/browsertests/* - Classes copied over from the same content/shell directory for testing use.
    • content/app/BUILD.gn, content/browser/BUILD.gn, content/child/BUILD.gn, content/common/BUILD.gn, content/gpu/BUILD.gn, content/renderer/BUILD.gn, content/utility/BUILD.gn, content/services/auction_worklet/public/mojom/BUILD.gn, media/mojo/clients/BUILD.gn, third_party/blink/public/common/BUILD.gn: Added //cobalt/testing/* to the visibility of various targets.

Future additions can include the content_unittests built on cobalt-specific changes and to include more tests in content browser tests that provide more coverage of the library.

Note: Not all tests pass. See test results - http://shortn/_dGJDarcvJY.

Test: out/android-arm_devel/bin/run_cobalt_browsertests -v
Fixed: 425936227

Awallky added 5 commits July 15, 2025 22:44
This adds the minimum framework for content browser tests to be run on cobalt's content_shell_lib and content_shell_app.

Future additions can include the content_unittests built on cobalt-specific changes and to include more tests in content browser tests that provide more coverage of the library.

Fixed: 425936227
All tests run, but they all seem to fail.
Adding cobalt browser test instance to build tooling.
@Awallky
Copy link
Contributor Author

Awallky commented Jul 16, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive framework for content browser tests, which is a significant step forward for our testing capabilities. The changes are extensive, primarily involving the addition of new files and build configurations to support the test environment on Android.

My review identified a few areas needing attention:

  • Critical: There are a couple of incorrect fully-qualified class names in Android layout XML files that will cause runtime crashes.
  • High: Several new C++ header files use incorrect include guards, which could lead to compilation issues. This seems to be a result of copy-pasting files without updating them for their new location.

Addressing these points will improve the correctness and maintainability of this new test framework. Great work on this large and important feature.

@Awallky
Copy link
Contributor Author

Awallky commented Jul 16, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request successfully adds the foundational framework for content browser tests in Cobalt, a crucial step for improving test coverage and decoupling from content/shell. The changes are extensive, involving numerous new files and build system modifications, which appear to be correctly adapted from their Chromium counterparts.

I've identified a few areas for improvement, including a critical memory leak in the test fixture, some uses of deprecated Android APIs, and minor opportunities to align with best practices for code maintainability and accessibility. Addressing these points will help ensure the new test framework is robust and easy to maintain.

@Awallky Awallky marked this pull request as ready for review July 16, 2025 18:45
@Awallky Awallky requested review from a team as code owners July 16, 2025 18:45
@Awallky Awallky requested a review from andrewsavage1 July 16, 2025 18:45
@haozheng-cobalt
Copy link
Contributor

Looks good in general to me.
Just 2 main nit issues:

  1. We should update all copyrights to Cobalt.
  2. and wrap Cobalt's logic in if(is_cobalt) for BUILD files in the upstream directories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants