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.

Sign up for GitHub

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

Jump to bottom

Unit tests for virtual grants #4344

Closed
wants to merge 1 commit into from
Closed

Unit tests for virtual grants #4344

wants to merge 1 commit into from

Conversation

masparrow
Copy link
Contributor

@masparrow masparrow commented Jan 9, 2020 •
edited
Loading

Resolves brave/brave-browser#6949

  • Bind Utils
  • Promotion
  • Promotion Utils
  • Security Helper (Ledger

Submitter Checklist:

Test Plan:

Nb. I have two failing unit tests which are unrelated to my changes, and they seem to be failing on master - otherwise all unit tests pass - (888/890);

2 tests crashed:
    MediaYouTubeTest.GetChannelUrl (../../brave/vendor/bat-native-ledger/src/bat/ledger/internal/media/youtube_unittest.cc:498)
    MediaYouTubeTest.GetVideoUrl (../../brave/vendor/bat-native-ledger/src/bat/ledger/internal/media/youtube_unittest.cc:466

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

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.

masparrow
masparrow commented Jan 9, 2020
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/promotion/promotion_unittest.cc
// void OnTimer(const uint32_t timer_id);
// Nb. Cannot test without mocking Promotion
// discussed with tmancey and no value as code may be changing
// need to confirm with nejc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NejcZdovc Thoughts here?

@masparrow masparrow requested review from a team and bridiver January 9, 2020 15:15
@masparrow masparrow self-assigned this Jan 9, 2020
@masparrow masparrow marked this pull request as ready for review January 9, 2020 15:17
tmancey
tmancey approved these changes Jan 9, 2020
View reviewed changes
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

@masparrow masparrow requested a review from a team January 14, 2020 16:31
tmancey
tmancey approved these changes Jan 22, 2020
View reviewed changes
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++

@NejcZdovc
Copy link
Contributor

CI is failing on unit tests https://ci.brave.com/job/brave-browser-build-pr/job/issues%252F6949/1/execution/node/591/log/

@masparrow
Copy link
Contributor Author

CI is failing on unit tests https://ci.brave.com/job/brave-browser-build-pr/job/issues%252F6949/1/execution/node/591/log/

Most failing are unrelated, but these two;

  • PromotionTest.TestRefreshWithRetryWithTimerId
  • PromotionTest.TestRefreshWithoutRetryWithTimerId

failed and are related, but passed locally. The only failing tests I had locally were the YouTube related Media ones.

I'll check it all out again, and rebase if needed.

@masparrow masparrow removed CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 2, 2020
NejcZdovc
NejcZdovc reviewed Mar 4, 2020
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/test/mock_ledger_client.h
@@ -29,6 +29,14 @@ class MockLedgerClient : public ledger::LedgerClient {

void CreateWallet(ledger::LedgerCallbackHandler* handler) override;

MOCK_METHOD6(LoadURL,
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this added here? we have ledger_client_mock.h where we define mocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's confusing naming then :D - but, this appear to be the 'mock' in use by the code, as this change resolved the failing tests (related to the discussion we had on zoom).

@masparrow masparrow force-pushed the issues/6949 branch 3 times, most recently from 563b173 to 1601086 Compare March 17, 2020 11:20
masparrow
masparrow commented Mar 17, 2020
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/promotion/promotion_unittest.cc
ledger::kStatePromotionCorruptedMigrated))
.WillByDefault(testing::Return(false));

EXPECT_CALL(*mock_ledger_impl_, GetAllPromotions(_)).Times(2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NejcZdovc promotion->Initialize() invokes the GetAllPromotions callback twice in this scenario, once to perform the CheckForCorrupted and then it falls through to invoke Retry... is this the expected behaviour? As it means CheckForCorrupted is invoking an async LoadURL, and then the code moves on to processing local data in Retry

Copy link
Contributor

Choose a reason for hiding this comment

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

we could make it sync just for double safety

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NejcZdovc do you want me to make that change as part of this PR? (I'd say it's a little out of scope tbh and is better as a new issue IMHO).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Ticket for GetAllPromotions rethink

Copy link
Contributor

Choose a reason for hiding this comment

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

new issue is fine

@masparrow masparrow closed this Mar 17, 2020
@masparrow masparrow deleted the issues/6949 branch March 17, 2020 17:20
@masparrow masparrow restored the issues/6949 branch March 17, 2020 17:49
@NejcZdovc NejcZdovc reopened this Mar 18, 2020
@masparrow masparrow added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 18, 2020
@masparrow
Unit tests for virtual grants
f377ce5 
Resolves brave/brave-browser#6949

- Bind Utils
- Promotion
- Promotion Utils
- Security Helper (Ledger)
@masparrow masparrow removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 19, 2020
@NejcZdovc NejcZdovc assigned NejcZdovc and unassigned masparrow Apr 9, 2020
@NejcZdovc NejcZdovc closed this Aug 19, 2020
@tmancey tmancey deleted the issues/6949 branch June 11, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@tmancey tmancey tmancey approved these changes

@gdregalo gdregalo gdregalo approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unit tests for virtual grants
4 participants
@masparrow @NejcZdovc @tmancey @gdregalo