Browser test fixes #23657

bridiver · 2024-05-14T21:35:44Z

Resolves brave/brave-browser#38325

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

bsclifton · 2024-05-16T23:36:09Z

browser/ai_chat/ai_chat_browsertests.cc

  }

-  void SetUpOnMainThread() override {
-    mock_cert_verifier_.mock_cert_verifier()->set_default_result(net::OK);
-    host_resolver()->AddRule("*:*", "127.0.0.1");


This was removed but not added back - does it need to be added above under SetUpOnMainThread?

bsclifton · 2024-05-16T23:50:22Z

browser/request_otr/request_otr_browsertest.cc

-    brave::RegisterPathProvider();
-
-    // Reuse upstream test files in content.
-    content::RegisterPathProvider();


was this weird behavior (registering our path provider... then content path provider) needed because paths were registered so many times? Really curious why that was added 🤔

all of the path registration stuff was a mess. It's fixed now

bsclifton

Building and running tests to try it out - but went over ever change, left some comments.

Changes LGTM. Super nice cleanup in chromium_src/chrome/app/chrome_main_delegate.cc! 😄

Also nice to have the override simplification in chromium_src/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

bsclifton · 2024-05-20T20:27:54Z

Probably already known - but I get a ton of crashes when running npm run test -- brave_unit_tests. I didn't try npm run test -- brave_browser_tests yet

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

iefremov · 2024-05-29T22:23:28Z

browser/ephemeral_storage/blob_url_browsertest.cc

@@ -71,11 +71,6 @@ class BlobUrlBrowserTestBase : public EphemeralStorageBrowserTest {
  };
  using FramesWithRegisteredBlobs = std::vector<RenderFrameHostBlobData>;

-  void SetUpOnMainThread() override {
-    EphemeralStorageBrowserTest::SetUpOnMainThread();
-    ASSERT_TRUE(embedded_test_server()->Start());


i don't follow why this is moved

I had to resolve several issues across tests that subclass EphemeralStorageBrowserTest so I moved it there to simplify things.

I don't recall the exact problem now, but I think maybe it needed be called before https://github.com/brave/brave-core/pull/23657/files/7091e291ca6d99689b310ff5aac503373442412b#diff-ad1b5bc543791a83f0e762f32a01e88544b826f9ecdad47ce2b8a49461cf7a7bR192

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

aseren

Brave Ads LGTM

github-actions · 2024-06-03T21:33:57Z

[puLL-Merge] - brave/brave-core@23657

Here is my review of the PR changes:

Description

This PR makes several changes related to how Brave registers custom path providers and sets up embedded test servers for browser tests. The main changes are:

Move the RegisterPathProvider function from brave_paths.cc to an override in chrome_paths.cc. This allows removing the separate brave_paths.cc file.
Remove calls to brave::RegisterPathProvider() from many browser test setup methods. Instead, move the test data directory logic into SetUpOnMainThread().
Remove the BraveCommandLineHelper class and its unit tests, moving the logic into BraveMainDelegate::BasicStartupComplete() directly.

The motivation seems to be simplifying the path provider registration and browser test setup code.

Changes

Changes

app/:
- Remove BraveCommandLineHelper class and tests
- Move logic from BraveCommandLineHelper into BraveMainDelegate::BasicStartupComplete()
- Define BraveMainDelegate::BasicStartupComplete() directly instead of using BasicStartupComplete_ChromiumImpl macro
browser/, components/:
- Remove many calls to brave::RegisterPathProvider() from browser tests
- Move test data directory logic from SetUp() to SetUpOnMainThread() in several browser tests
- Fix a few test server setup issues
chromium_src/chrome/app/:
- Remove unused includes and macro logic from chrome_main_delegate.cc
- Include brave_main_delegate.cc directly
chromium_src/chrome/common/:
- Move RegisterPathProvider to an override in chrome_paths.cc
components/constants/:
- Delete brave_paths.cc, moving functionality to chrome_paths.cc
- Update comments in brave_paths.h

Let me know if you have any other questions!

bridiver requested review from iefremov, a team and simonhong as code owners May 14, 2024 21:35

github-actions bot added the CI/run-network-audit Run network-audit label May 14, 2024

github-actions bot assigned bridiver May 14, 2024

github-actions bot added the puLL-Merge label May 14, 2024

fmarier approved these changes May 14, 2024

View reviewed changes

bridiver requested a review from a team as a code owner May 14, 2024 22:50

bsclifton reviewed May 16, 2024

View reviewed changes

bsclifton approved these changes May 16, 2024

View reviewed changes

bridiver force-pushed the browser-test-fixes branch from 224c009 to 047ca37 Compare May 20, 2024 22:48

iefremov reviewed May 29, 2024

View reviewed changes

browser/ephemeral_storage/ephemeral_storage_browsertest.cc Show resolved Hide resolved

iefremov reviewed May 29, 2024

View reviewed changes

browser/ephemeral_storage/ephemeral_storage_browsertest.cc Outdated Show resolved Hide resolved

iefremov approved these changes May 29, 2024

View reviewed changes

darkdh approved these changes May 30, 2024

View reviewed changes

simonhong approved these changes May 30, 2024

View reviewed changes

aseren approved these changes May 30, 2024

View reviewed changes

bridiver added 5 commits June 3, 2024 14:32

fix delegate/path provider

26e7399

fix incorrect test setup

6258269

remove brave::RegisterPathProvider from unit test

c34b98d

presubmit fixes

2bddac7

use Brave delegates by default for all tests

52a8ed2

bridiver added 24 commits June 3, 2024 14:32

cleanup

f34a7a9

don't forget to call superclass

c2e2109

fixup

13e6b08

presubmit fixes

b0f6fa2

kHostResolverRules fixes

f58def0

remove method declaration

0545153

presubmit fixes

a6f27bf

add handlers before starting

8a687b7

more test setup fixes

4ca75d9

fix url

f48fc69

presubmit fixes

39169ee

render view overrides break on upstream tests

da9b8e5

fix url rewriter tests

27fa5d0

start embedded test server

3c1127f

fix pattern/url match

1f2c566

fix request-otr interstitial test

02a16de

add host resolver

1f18110

fix path

1e6be95

re-add missing BraveContentClient override

d7ed5b4

fix console message

21b5679

disable upstream amp ukm tests

cc9ccad

fixing unit test content client breaks upstream tests

912072c

review comments

69395a8

undo = default

42ef212

bridiver force-pushed the browser-test-fixes branch from bf7b339 to 42ef212 Compare June 3, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser test fixes #23657

Browser test fixes #23657

bridiver commented May 14, 2024 •

edited

bsclifton May 16, 2024

bsclifton May 16, 2024

bridiver May 28, 2024

bsclifton left a comment •

edited

bsclifton commented May 20, 2024

iefremov May 29, 2024

bridiver May 29, 2024 •

edited

bridiver May 29, 2024

aseren left a comment

github-actions bot commented Jun 3, 2024

Changes

Browser test fixes #23657

Are you sure you want to change the base?

Browser test fixes #23657

Conversation

bridiver commented May 14, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton May 16, 2024

Choose a reason for hiding this comment

bsclifton May 16, 2024

Choose a reason for hiding this comment

bridiver May 28, 2024

Choose a reason for hiding this comment

bsclifton left a comment • edited

Choose a reason for hiding this comment

bsclifton commented May 20, 2024

iefremov May 29, 2024

Choose a reason for hiding this comment

bridiver May 29, 2024 • edited

Choose a reason for hiding this comment

bridiver May 29, 2024

Choose a reason for hiding this comment

aseren left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 3, 2024

Description

Changes

bridiver commented May 14, 2024 •

edited

bsclifton left a comment •

edited

bridiver May 29, 2024 •

edited