Skip to content
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

[Brave News]: Don't use TestingProfile in component unittests #25914

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

fallaciousreasoning
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning commented Oct 9, 2024

Resolves brave/brave-browser#41534

Waiting on #25911 to merge first merged!

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:

Test Plan:

N/A

@fallaciousreasoning
Copy link
Contributor Author

Note: This needs to merge AFTER #25911

@fallaciousreasoning fallaciousreasoning changed the title [Brave News]: Don't use TestingProfile in unittests [Brave News]: Don't use TestingProfile in component unittests Oct 10, 2024
@fallaciousreasoning fallaciousreasoning requested review from goodov, simonhong and petemill and removed request for goodov October 15, 2024 02:46
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++
optional: imo, test/BUILD.gn should be in same directory with test files.

Copy link
Contributor

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

Description

This PR refactors the test setup for Brave News components, removing dependencies on Chrome-specific testing infrastructure and replacing them with more focused, component-specific test utilities. The changes aim to improve the modularity and independence of the Brave News tests.

Changes

Changes

  1. components/brave_news/browser/DEPS:

    • Removed specific include rules for unit tests, simplifying the dependencies.
  2. components/brave_news/browser/brave_news_pref_manager_unittest.cc:

    • Replaced TestingProfile with sync_preferences::TestingPrefServiceSyncable.
    • Updated test setup to use a standalone PrefService instead of relying on a full Profile.
  3. components/brave_news/browser/channels_controller_unittest.cc:

    • Similar changes as in brave_news_pref_manager_unittest.cc.
    • Replaced Profile-based preference management with direct PrefService usage.
  4. components/brave_news/browser/direct_feed_controller_unittest.cc:

    • Removed TestingProfile dependency.
    • Simplified test setup by removing unnecessary Profile-related code.
  5. components/brave_news/browser/feed_building_unittest.cc:

    • Replaced TestingProfile with sync_preferences::TestingPrefServiceSyncable.
    • Updated test setup to use standalone PrefService.
  6. components/brave_news/browser/initialization_promise_unittest.cc:

    • Similar changes as in other test files, replacing Profile-based setup with PrefService-based setup.
  7. components/brave_news/browser/publishers_controller_unittest.cc:

    • Replaced TestingProfile with sync_preferences::TestingPrefServiceSyncable.
    • Updated test setup to use standalone PrefService and BraveNewsPrefManager.
  8. components/brave_news/browser/suggestions_controller_unittest.cc:

    • Similar changes as in other test files, replacing Profile-based setup with PrefService-based setup.
  9. components/brave_news/browser/test/BUILD.gn:

    • Updated dependencies to remove Chrome-specific test support.
    • Added more focused dependencies for testing components like prefs, sync_preferences, and network.

These changes collectively move the test infrastructure away from Chrome-specific components and towards more modular, component-specific testing utilities. This should make the tests more focused and easier to maintain, as well as reducing dependencies on Chrome internals.

@fallaciousreasoning
Copy link
Contributor Author

@simonhong agreed, but I'd rather do it as a separate PR if that's okay

@fallaciousreasoning fallaciousreasoning enabled auto-merge (squash) October 15, 2024 20:34
@bridiver
Copy link
Collaborator

++ optional: imo, test/BUILD.gn should be in same directory with test files.

Huh, that is a bit weird. It could go either way, move the tests into the test directory or move the test target out of it, but follow up is fine

@fallaciousreasoning fallaciousreasoning merged commit b3602aa into master Oct 15, 2024
17 checks passed
@fallaciousreasoning fallaciousreasoning deleted the bn-test-no-profile branch October 15, 2024 21:30
@github-actions github-actions bot added this to the 1.73.x - Nightly milestone Oct 15, 2024
@brave-builds
Copy link
Collaborator

Released in v1.73.14

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

Successfully merging this pull request may close these issues.

[Brave News]: Don't use TestingProfile in unittests - its a deps violation
4 participants