Fix interactive UI tests build #25841

Ilie-Lesan · 2024-10-07T08:30:22Z

Resolves: brave/brave-browser#41436

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:

goodov · 2024-10-08T15:30:34Z

+ npm run presubmit
Running presubmit upload checks ...
  checking for commit objects in tree took a long time: 0.6s
** Presubmit Messages: 1 **
-------------------------
[Info] chromium_src:
Located 12 #define statements.
** Presubmit ERRORS: 1 **
The brave directory requires source formatting. Please run: npm run presubmit -- --fix.
--- chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc  (before formatting)
+++ chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc  (after formatting)
@@ -3,5 +3,6 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at https://mozilla.org/MPL/2.0/. */
+#include "src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc"
+
 #include "brave/browser/ui/views/tabs/brave_tab_hover_card_controller.h"
-#include "src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc"
Presubmit checks took 2.6s to calculate.
There were presubmit errors.
null
Process exited with code 1

Ilie-Lesan · 2024-10-08T17:46:12Z

@goodov Fixed. Thanks!

goodov · 2024-10-09T06:52:55Z

@goodov Fixed. Thanks!

ah, you tried to fix the build failure by adding a specific header, but the include ordering wanted the different order, I missed that, sorry. One of the option to keep the order of includes is to add a comment before the #include line. This way the reordering shouldn't happen.

If you want to restore the original impl, but add a comment to keep the includes order exact, please do, otherwise we can go with a define.

Ilie-Lesan · 2024-10-09T18:34:57Z

@goodov Fixed. Thanks!

ah, you tried to fix the build failure by adding a specific header, but the include ordering wanted the different order, I missed that, sorry. One of the option to keep the order of includes is to add a comment before the #include line. This way the reordering shouldn't happen.

If you want to restore the original impl, but add a comment to keep the includes order exact, please do, otherwise we can go with a define.

I think is ok to keep it as it is right now.

goodov · 2024-10-10T08:52:29Z

well, now we have this. maybe restoring to the includes approach will be better.

pls run npm run presubmit locally to make sure nothing else pops up.

+ npm run presubmit
Running presubmit upload checks ...
  checking for commit objects in tree took a long time: 0.6s
** Presubmit Messages: 1 **
-------------------------
[Info] chromium_src:
Located 13 #define statements.
** Presubmit ERRORS: 1 **
-------------------------
[Error] chromium_src:
  Override chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc
  defines symbol TabHoverCardController but the symbol could not be found in
  /home/ubuntu/workspace/teamcity/work/contributor-Ilie-Lesan-fix-interactive-ui-tests-build/src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc
  and is not used internally in the override.
Presubmit checks took 2.5s to calculate.
There were presubmit errors.
null
Process exited with code 1

Ilie-Lesan · 2024-10-10T13:15:21Z

well, now we have this. maybe restoring to the includes approach will be better.

pls run npm run presubmit locally to make sure nothing else pops up.

+ npm run presubmit
Running presubmit upload checks ...
  checking for commit objects in tree took a long time: 0.6s
** Presubmit Messages: 1 **
-------------------------
[Info] chromium_src:
Located 13 #define statements.
** Presubmit ERRORS: 1 **
-------------------------
[Error] chromium_src:
  Override chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc
  defines symbol TabHoverCardController but the symbol could not be found in
  /home/ubuntu/workspace/teamcity/work/contributor-Ilie-Lesan-fix-interactive-ui-tests-build/src/chrome/browser/ui/views/tabs/tab_hover_card_controller_interactive_uitest.cc
  and is not used internally in the override.
Presubmit checks took 2.5s to calculate.
There were presubmit errors.
null
Process exited with code 1

I think I've run the npm run presubmit -- --fix command locally before pushing and everything looked fine. I will add the old impl(with the comments added) anyway. Thanks!

goodov · 2024-10-15T14:18:42Z

chromium_src/chrome/browser/privacy_sandbox/browsing_topics_settings_interactive_uitest.cc

+#define BlockFirstTopicOnManageTopicsPage \
+  DISABLED_BlockFirstTopicOnManageTopicsPage
+#define UnblockOneTopicOnAdTopicsPage DISABLED_UnblockOneTopicOnAdTopicsPage
+#define ConfirmDefaultIconIsNotUsed DISABLED_ConfirmDefaultIconIsNotUsed


sorry, I don't see how these defines are related to tests build.

we have *.filter files to disable upstream tests, so we shouldn't disable them via defines here.

Since the kPrivacySandboxManageTopicsURL and kPrivacySandboxAdTopicsURL pages are not available in Brave, and those added by me are just for fixing the build, I wanted to disable these tests. Disabling the tests via a config file is much better. Thanks!

pls remove these defines, let's only fix the build with your PR. I'd like to merge it, but this is not something we should have here.

@goodov Done. Thanks!

mihaiplesa · 2025-10-04T07:28:25Z

Should this be closed or still of interest?

goodov · 2025-10-06T09:46:38Z

we don't run interactive tests, so it is non priority for us. If the author wants this to be landed, then it needs to be rebased and updated to match our current chromium_src overrides.

Ilie-Lesan added 2 commits October 6, 2024 23:28

Fix interactive ui tests build

1d97c8f

Add license header

a4e44e7

Ilie-Lesan requested a review from a team as a code owner October 7, 2024 08:30

brave-builds mentioned this pull request Oct 8, 2024

CI run for contributor PR #25841 #25882

Draft

Find a better way to fix the dependency

83a1fc1

Silence pre-submit warnings

aa20d84

goodov reviewed Oct 15, 2024

View reviewed changes

Don't disable chromium tests

07980b6

mihaiplesa assigned Ilie-Lesan Oct 4, 2025

Fix interactive UI tests build #25841

Are you sure you want to change the base?

Fix interactive UI tests build #25841

Uh oh!

Conversation

Ilie-Lesan commented Oct 7, 2024

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Uh oh!

goodov commented Oct 8, 2024

Uh oh!

Ilie-Lesan commented Oct 8, 2024

Uh oh!

goodov commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilie-Lesan commented Oct 9, 2024

Uh oh!

goodov commented Oct 10, 2024

Uh oh!

Ilie-Lesan commented Oct 10, 2024

Uh oh!

goodov Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

Ilie-Lesan Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

goodov Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Ilie-Lesan Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

mihaiplesa commented Oct 4, 2025

Uh oh!

goodov commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

goodov commented Oct 9, 2024 •

edited

Loading