Bytecode tests for static methods #23699

AlexeyBarabash · 2024-05-16T13:54:24Z

This PR expands BytecodeTest.methodExists test to detect change of method modifier.
Also this PR removes unnecessary tests against Brave*** classes, because we own that code and it will be changed by us if we need after the upstream changes.

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:

npm run test -- brave_unit_tests Release --target_os=android --target_arch=x86

AlexeyBarabash · 2024-05-17T10:53:51Z

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

+                        false,
+                        null));
+
+        // Why do we have this below? we can guarantee whatever we need at Brave code


@samartnik , I forgot why do we need the methodExists tests for Brave***** classes. This looks suspicious here and below in several places. Can you please remind me why we did so?

yes, this seems to be overkill, the main point of these tests is to detect changes in the upstream

thanks for the confirmation @samartnik , I am updating the PR.

samartnik

lgtm

deeppandya

LGTM

SergeyZhukovsky

++

github-actions · 2024-05-21T17:47:31Z

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

Description

This PR makes changes to the Android bytecode test to remove redundant checks for Brave-specific classes and methods. It also adds a few new checks for methods in existing Chromium classes.

Changes

Changes

android/java/apk_for_test.flags

Removes redundant entries for Brave-specific classes and methods that no longer need to be checked separately. This includes classes like BraveBookmarkUtils, BravePermissionDialogModel, BraveTabUiThemeProvider, etc.
Adds a few new entries for methods in existing Chromium classes that need to be checked, such as SiteSettingsCategory.contentSettingsType() and SiteSettingsCategory.preferenceKey()

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

Adds an enum MethodModifier to distinguish between static and regular methods when checking if a method exists.
Updates the testClassesExist() method to remove checks for redundant Brave-specific classes.
Updates the testMethodsExist() and testMethodsForInvocationExist() methods:
- Removes checks for redundant Brave-specific methods
- Adds the MethodModifier parameter when calling methodExists() to specify whether the method being checked should be static or not
- Adds a few new checks for methods like SiteSettingsCategory.contentSettingsType() and SiteSettingsCategory.preferenceKey()
Modifies the methodExists() helper method to take the new MethodModifier parameter and check the method's actual modifier against the expected modifier.

Overall, this PR cleans up the bytecode test by removing unnecessary checks for Brave-specific classes and methods, and adds a few missing checks for existing Chromium code. The new MethodModifier helps catch bugs where a method's static/non-static declaration may have changed incorrectly.

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

* [CodeHealth] Use span for args on `redirect_cc` (#23698) This change corrects the build errors when building `redirect_cc` with `-Wunsafe-buffers-usage`, by introducing a span argument to handle access to the array of strings in `argv`. The resulting code is more readable, and also relies on the hardening features available to span. * Bytecode tests for static methods (#23699) * Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests * Merge pull request #23233 from brave/cr126 Upgrade from Chromium 125 to Chromium 126 * Fix test_data dependency. (#23841) * Merge pull request #23862 from brave/search_promotion_layout Fixed search promotion's layout regression * Merge pull request #23869 from brave/cr126-followup-fix-share-this-page-icon Use Brave branded icon for Share this page * Merge pull request #23868 from brave/cr126-followup-fix-settings-icons Fix use of upstream icons in brave://settings * Merge pull request #23871 from brave/cr126-followup-disable-pwa-universal-install Disable PWA universal install feature flag * Merge pull request #23874 from brave/cr126-followup-disable-security-privacy-feature-flags Disable features flagged by privacy/security team * Upgrade from Chromium 126.0.6478.17 to Chromium 126.0.6478.26 (#23896) * Update from Chromium 126.0.6478.17 to Chromium 126.0.6478.26. * Conflict-resolved patches from Chromium 126.0.6478.17 to Chromium 126.0.6478.26. * Update patches from Chromium 126.0.6478.17 to Chromium 126.0.6478.26. * Updated strings for Chromium 126.0.6478.26. * Merge pull request #23897 from brave/android_search_widget_crash Fixes a crash on search widget on Android * Merge pull request #23894 from brave/cr126-followup-disable-sync-promo Disable showing sync promo * Merge pull request #23902 from brave/cr126-followup-fix-dangerous-downloads-ui Disable safe_browsing::kImprovedDownloadPageWarnings feature flag * Merge pull request #23909 from brave/help_bubble_view_button_regressions Fixed HelpBubbleView's secondary button text is not visible * Fixed color of switch at Shields and content filters (#23910) Fixed color of switch at Shields and content filters --------- Co-authored-by: cdesouza-chromium <cdesouza@brave.com> Co-authored-by: AlexeyBarabash <AlexeyBarabash@users.noreply.github.com> Co-authored-by: goodov <5928869+goodov@users.noreply.github.com> Co-authored-by: Simon Hong <shong@brave.com> Co-authored-by: Serg <serg.zhukovsky@gmail.com>

AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label May 16, 2024

github-actions bot assigned AlexeyBarabash May 16, 2024

AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label May 17, 2024

AlexeyBarabash marked this pull request as ready for review May 17, 2024 10:50

AlexeyBarabash requested review from deeppandya, SergeyZhukovsky and samartnik May 17, 2024 10:51

AlexeyBarabash commented May 17, 2024

View reviewed changes

AlexeyBarabash force-pushed the bytecode_test_static_methods branch from 440b607 to 473ece1 Compare May 17, 2024 10:56

AlexeyBarabash marked this pull request as draft May 20, 2024 17:30

AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label May 20, 2024

AlexeyBarabash force-pushed the bytecode_test_static_methods branch from 125dd92 to a7cadc4 Compare May 20, 2024 18:33

AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label May 20, 2024

AlexeyBarabash force-pushed the bytecode_test_static_methods branch 2 times, most recently from a0c5e16 to 20c7853 Compare May 21, 2024 09:10

AlexeyBarabash marked this pull request as ready for review May 21, 2024 15:23

samartnik approved these changes May 21, 2024

View reviewed changes

deeppandya approved these changes May 21, 2024

View reviewed changes

SergeyZhukovsky approved these changes May 21, 2024

View reviewed changes

AlexeyBarabash added 5 commits May 21, 2024 20:34

Bytecode tests for static methods

cb22955

Static/regular method is now arg of methodExists

d60f4a8

Removed Brave**** classes from methodExists tests

74607bc

Removed Brave**** classes from classExists tests

c8493f5

fixed formatting

fbd5859

AlexeyBarabash force-pushed the bytecode_test_static_methods branch from 20c7853 to fbd5859 Compare May 21, 2024 17:46

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

AlexeyBarabash merged commit acee0aa into master May 21, 2024
19 checks passed

AlexeyBarabash deleted the bytecode_test_static_methods branch May 21, 2024 19:25

github-actions bot added this to the 1.68.x - Nightly milestone May 21, 2024

emerick pushed a commit that referenced this pull request May 24, 2024

Bytecode tests for static methods (#23699)

98aee61

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

emerick mentioned this pull request May 24, 2024

Upgrade from Chromium 125 to Chromium 126 (1.67.x) #23832

Merged

24 tasks

emerick pushed a commit that referenced this pull request May 24, 2024

Bytecode tests for static methods (#23699)

5b5cd8e

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

emerick pushed a commit that referenced this pull request May 25, 2024

Bytecode tests for static methods (#23699)

b7ab546

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

emerick pushed a commit that referenced this pull request May 25, 2024

Bytecode tests for static methods (#23699)

84ad7d5

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

emerick pushed a commit that referenced this pull request May 29, 2024

Bytecode tests for static methods (#23699)

8215ac0

* Static/regular method is now arg of methodExists * Removed Brave**** classes from methodExists/classExists tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bytecode tests for static methods #23699

Bytecode tests for static methods #23699

AlexeyBarabash commented May 16, 2024 •

edited

AlexeyBarabash May 17, 2024

samartnik May 17, 2024

AlexeyBarabash May 17, 2024

samartnik left a comment

deeppandya left a comment

SergeyZhukovsky left a comment

github-actions bot commented May 21, 2024

Changes

android/java/apk_for_test.flags

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

Bytecode tests for static methods #23699

Bytecode tests for static methods #23699

Conversation

AlexeyBarabash commented May 16, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

AlexeyBarabash May 17, 2024

Choose a reason for hiding this comment

samartnik May 17, 2024

Choose a reason for hiding this comment

AlexeyBarabash May 17, 2024

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2024

Description

Changes

android/java/apk_for_test.flags

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

AlexeyBarabash commented May 16, 2024 •

edited