Skip to content

Testable BatchResponse - #389

Merged
lahirumaramba merged 3 commits into
firebase:masterfrom
ToxicBakery:feature/BatchResponseImpl
Apr 10, 2020
Merged

Testable BatchResponse#389
lahirumaramba merged 3 commits into
firebase:masterfrom
ToxicBakery:feature/BatchResponseImpl

Conversation

@ToxicBakery

Copy link
Copy Markdown
Contributor

Resolves #388

  • Converts batch response to an interface
  • Moves batch response implementation to BatchResponseImpl
  • Update FirebaseMessagingClientImpl to use BatchResponseImpl

- Converts batch response to an interface
- Moves batch response implementation to BatchResponseImpl
- Update FirebaseMessagingClientImpl to use BatchResponseImpl
- Updated tests

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks reasonable to me with just one change suggested.

@lahirumaramba what do you think?

Comment thread src/main/java/com/google/firebase/messaging/BatchResponseImpl.java Outdated
@hiranya911

Copy link
Copy Markdown
Contributor

Hey @ToxicBakery. I just pushed a change to our CI config, so that they will be run on external contributions such as this PR. Can I ask you to please sync against the latest master branch, and make a push?

@ToxicBakery

Copy link
Copy Markdown
Contributor Author

Done, appears to have run successfully 👍

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@lahirumaramba I'll leave this to you to review and merge.

@lahirumaramba lahirumaramba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thank you!

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.

BatchResponse implementation makes testing difficult.

3 participants