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

replaced a if check by assert statement #23876

Conversation

ajayseeker
Copy link
Contributor

@ajayseeker ajayseeker commented May 28, 2024

Resolves brave/brave-browser#29918

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:

@ajayseeker
Copy link
Contributor Author

Hi @sangwoo108!
Created this PR, I replaced the if statement with assert, build ran fine and browser too launched without any glitch. I guess we now need to check if all tests pass with this change to be sure that this isn't breaking, right?

if (toolkit_views) {
deps += [ "//brave/components/sidebar/browser" ]
}
deps += [ "//brave/components/sidebar/browser" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this into deps = [] above. We don't need to add this with +=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@sangwoo108
Copy link
Contributor

Hi @sangwoo108! Created this PR, I replaced the if statement with assert, build ran fine and browser too launched without any glitch. I guess we now need to check if all tests pass with this change to be sure that this isn't breaking, right?

That's right. I'm going to run CI 😄

@sangwoo108
Copy link
Contributor

CI complains about the formatting. Please run npm run format

@ajayseeker
Copy link
Contributor Author

CI complains about the formatting. Please run npm run format

oh sorry! should have taken care of this.
I ran npm run format and checked-in change. Can we run CI once again?
@sangwoo108

@ajayseeker
Copy link
Contributor Author

Security check seems to fail, but looking at the error

securityUnhandled error: HttpError: Resource not accessible by integration
Unhandled error: HttpError: Resource not accessible by integration

it doesn't seem related. @sangwoo108, what went wrong?

@sangwoo108
Copy link
Contributor

Yeah, looks weird. Asking other folks.

@sangwoo108 sangwoo108 force-pushed the issues/29918/try-adding-assert-toolkit_views branch from 6790f78 to 5383a73 Compare May 29, 2024 07:16
@ajayseeker
Copy link
Contributor Author

Yeah, looks weird. Asking other folks.

Hey! the tests passed, I guess we just need someone to review it and then it can be merged.

Copy link
Contributor

@sangwoo108 sangwoo108 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the patch

@sangwoo108 sangwoo108 merged commit 7b714db into brave:master May 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try adding assert(toolkit_views) to extensions/BUILD.gn
2 participants