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

Adds delays for suggestion api (uplift to 1.3) #4448

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Jan 28, 2020

A more concise manual merge resolution of #4441

Uplift of #4372
Fixes brave/brave-browser#7709
Fixes brave/brave-browser#6940

Approved, please ensure that before merging:

  • You have checked CI and the builds, lint, and tests all pass or are not related to your PR.
  • You have tested your change on Nightly.
  • The PR milestones match the branch they are landing to.

After you merge:

  • The associated issue milestone is set to the smallest version that the changes is landed on.

Adds delays for suggestion api
@bsclifton bsclifton changed the title Merge pull request #4372 from brave/ac-delay WIP: Adds delays for suggestion api (uplift to 1.3) Jan 28, 2020
@bsclifton bsclifton added this to the 1.3.x - Beta milestone Jan 28, 2020
@bsclifton bsclifton self-assigned this Jan 28, 2020
@bsclifton bsclifton changed the title WIP: Adds delays for suggestion api (uplift to 1.3) Adds delays for suggestion api (uplift to 1.3) Jan 28, 2020
@bsclifton
Copy link
Member Author

@tmancey can you help me check this out? I think we're good to go here... it builds, unit tests all pass. I only pulled in the minimum required

Once we verify it, we can assign to @brave/uplift-approvers for consideration 😄

@tmancey
Copy link
Collaborator

tmancey commented Jan 28, 2020

@bsclifton LGTM++

@bsclifton
Copy link
Member Author

bsclifton commented Jan 28, 2020

Might still be some additional work. Build worked fine, unit tests all pass. But looks like browser tests have a failure:
https://ci.brave.com/job/brave-browser-build-pr/job/suggestion-api-1.3_2nd_try/1/execution/node/596/log/

2 tests failed:
BraveRewardsBrowserTest.AutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2189)
BraveRewardsBrowserTest.ProcessPendingContributions (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2717)
6 tests crashed:
BraveRewardsBrowserTest.MultipleRecurringOverBudgetAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:3132)
BraveRewardsBrowserTest.RecurringAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:3085)
BraveRewardsBrowserTest.RecurringTipForVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2313)
BraveRewardsBrowserTest.TipConnectedPublisherAnon (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2940)
BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2960)
BraveRewardsBrowserTest.TipVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2274)

module ledger.mojom;

enum ContributionStep {
STEP_AC_TABLE_EMPTY = -4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask why start steps at -4?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea- the original PR does this (this is just an uplift)
#4372

Maybe @tmancey can provide more context?

@bsclifton
Copy link
Member Author

OK problem resolved- force pushed change after verifying all tests pass locally! 🎉

@bsclifton bsclifton requested a review from a team January 29, 2020 00:17
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

@kjozwiak
Copy link
Member

Looks like test-install failed on macOS as per https://ci.brave.com/job/brave-browser-build-pr/job/suggestion-api-1.3_2nd_try/2/execution/node/698/log/ which is a known issue that's not related to this specific PR. All the other platforms passed without any errors/failures.

Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

Uplift into 1.3.x approved after deliberating with @brave/uplift-approvers. macOS failed due to test-install which is a known problem as per #4448 (comment). PR was also checked on Nightly by @NejcZdovc and @LaurenWags as per #4372 (comment).

Please make sure that all the correct labels are being used and the associated issue is moved into the correct milestone.

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.

4 participants