-
Notifications
You must be signed in to change notification settings - Fork 879
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
Fix build of upstream Chromium's unit_tests & browser_tests targets #7899
Conversation
@bridiver and/or @mkarolin PTAL to this small PR to enable building the unit_tests binary, so that we can resume work on brave/brave-browser#8376. Thanks! |
71e222f
to
b984eae
Compare
b984eae
to
905efc4
Compare
f6b569a
to
6ff3798
Compare
FWIW, the failure in the linux CI bot is unrelated to this change (i.e. a pre-existing issue on
|
@bridiver @iefremov Pinging you on this again, thanks! Note: this patch is a dependency of both brave/brave-browser#8376 and brave/brave-browser#8297 |
patches/chrome-test-BUILD.gn.patch
Outdated
"//ui/web_dialogs:web_dialogs_unittests", | ||
"//v8", | ||
] | ||
+ deps += [ "//brave/test:brave_test_support_unit", ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to fix a linking error that happens because of the push_messaging_service_unittest.cc
having a dependency on BraveTestingProfile
(via a chromium_src override) that needs to be satisfied.
Without this bit, the following error happens at link time:
ld.lld: error: undefined symbol: BraveTestingProfile::BraveTestingProfile()
>>> referenced by push_messaging_service_unittest.cc:73 (/home/mario/work/brave-browser/src/brave/chromium_src/chrome/browser/push_messaging/../../../../../chrome/browser/push_messaging/push_messaging_service_unittest.cc:73)
>>> obj/chrome/test/unit_tests/push_messaging_service_unittest.o:((anonymous namespace)::PushMessagingTestingProfile::PushMessagingTestingProfile())
>>> did you mean: BraveTestingProfile::~BraveTestingProfile()
>>> defined in: obj/chrome/test/unit_tests/push_messaging_service_unittest.o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe it's best to insert brave_testing_profile into the testing_profile .h/.cc through chromium_src overrides and then they'll be available here without any patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize I could do that, thanks for the suggestion.
PR updated!
39c9f51
to
34824cc
Compare
Three build failures are currently preventing us from building these unit_tests & browser_test binaries from upstream: 1. A compilation error due to FakeDeviceInfoTracker not implementing Brave-specific's GetAllBraveDeviceInfo() and DeleteDeviceInfo() virtual methods from DeviceInfoTracker (added via an override). 2. A compilation error in //c/b/apps/guest_view/web_view_browsertest.cc because of testing code using RenderViewContextMenu's static method RegisterMenuShownCallbackForTesting(), which would expect a callback |base::OnceCallback<void(RenderViewContextMenu_Chromium*>| because of the source override in chromium_src/c/b/renderer_context_menu, but a |base::OnceCallback<void(BraveRenderViewContextMenu*)>| will be passed instead due to the redefinition set in that source override. 3. A linking error due to push_messaging_service_unittest.cc requiring a dependency for BraveTestingProfile (also added via an override). This patches fixes all those three errors via chromium overrides. Fix brave/brave-browser#14001
Three build failures are currently preventing us from building these
unit_tests & browser_test binaries from upstream:
A compilation error due to FakeDeviceInfoTracker not implementing
Brave-specific's GetAllBraveDeviceInfo() and DeleteDeviceInfo()
virtual methods from DeviceInfoTracker (added via an override).
A compilation error in //c/b/apps/guest_view/web_view_browsertest.cc
because of testing code using RenderViewContextMenu's static method
RegisterMenuShownCallbackForTesting(), which would expect a callback
|base::OnceCallback<void(RenderViewContextMenu_Chromium*>| because
of the source override in chromium_src/c/b/renderer_context_menu, but
a |base::OnceCallback<void(BraveRenderViewContextMenu*)>| will be
passed instead due to the redefinition set in that source override.
A linking error due to push_messaging_service_unittest.cc requiring
a dependency for BraveTestingProfile (also added via an override).
This patches fixes all those three errors via chromium overrides and
a small patch to //chrome/test/BUILD.gn to add a missing dependency.
Fix brave/brave-browser#14001
Resolves brave/brave-browser#14001
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed).Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on.
Test Plan:
N/A