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

Provide test address books via DI #1310

Open
wants to merge 12 commits into
base: main-ose
Choose a base branch
from

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Feb 13, 2025

Purpose

At the moment test address books are provided via a static companion object method which requires context to be passed in. Passing context to static methods is an antipattern, since the global accessibility of context introduces testing issues and hinders maintainability.

Short description

  • Create LocalTestAddressBookProvider which can be injected in test classes to provide LocalTestAddressBooks
  • Provide and use test address books in a "provide" method which ensures we always start with a clean slate. It deletes any old testing address books before creating the new testing address book which it cleans beforehand (removes contacts).
  • Move dirty check methods from LocalTestAddressBook to LocalGroupTest, since that's the only place they are used
  • remove unused readOnly flag

Note: We should probably not completely remove LocalTestAddressBook since it still gives us an easy way to set the group method for tests - which is a valid use case in my eyes. I don't think using a LocalAddressBook interface or mocking all the group method calls just to get rid of LocalTestAddressBook are very beneficial.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@sunkup sunkup added the refactoring Internal improvement of existing functions label Feb 13, 2025
@sunkup sunkup requested a review from ArnyminerZ February 13, 2025 14:38
@sunkup sunkup self-assigned this Feb 13, 2025
@sunkup sunkup force-pushed the 1305-provide-one-preferred-way-ideally-with-di-of-providing-test-address-books branch from 61d8ba9 to 2802ec6 Compare February 17, 2025 09:05
ArnyminerZ
ArnyminerZ previously approved these changes Feb 18, 2025
Copy link
Member

@ArnyminerZ ArnyminerZ left a comment

Choose a reason for hiding this comment

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

Looks good to me

@rfc2822 rfc2822 force-pushed the 1305-provide-one-preferred-way-ideally-with-di-of-providing-test-address-books branch from 4a47734 to 89a8f84 Compare February 20, 2025 15:55
@rfc2822 rfc2822 force-pushed the 1305-provide-one-preferred-way-ideally-with-di-of-providing-test-address-books branch from 73e258d to dc5abdd Compare February 20, 2025 16:18
@rfc2822
Copy link
Member

rfc2822 commented Feb 20, 2025

But very nice 👍🏻

@sunkup sunkup requested a review from rfc2822 February 24, 2025 09:58
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Minor things

account: Account,
provider: ContentProviderClient,
groupMethod: GroupMethod = GroupMethod.GROUP_VCARDS,
provideLocalTestAddressBook: (LocalTestAddressBook) -> Unit
Copy link
Member

Choose a reason for hiding this comment

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

The argument name confused me a bit because it looks like it does something (provide an address book). But it's only the block to be called. I think it's common in Kotlin to call this

  • block, like here
  • or action, like here.

// remove address book account / address book
assertTrue(accountManager.removeAccountExplicitly(
// recreate account of provided address book, since the account might have been renamed
Account(addressBook.addressBookAccount.name, addressBook.addressBookAccount.type)
Copy link
Member

Choose a reason for hiding this comment

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

Optional: maybe explicitly define val renamedAccount = and then delete that one for clarity.

Would reduce complexity of the call, but as you like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide one preferred way (ideally with DI) of providing test address books
3 participants