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

Changes for proposal "Mocking Centrifuge Client for Testing Without a Server" #113

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shalom-aviv
Copy link
Contributor

Pull Request Description

This Pull Request introduces changes aimed at improving the testability and flexibility of the Centrifuge client for iOS, written in Swift. The main focus is on introducing protocols for key entities and optimizing their usage. Additional information is here Proposal: Mocking Centrifuge Client for Testing Without a Server

Key Changes:

  1. Protocols for Core Classes:

    • Introduced the Client protocol for the CentrifugeClient class.
    • Introduced the ClientSubscription protocol for the CentrifugeSubscription class.
    • These protocols replicate the current interfaces of their respective classes, enabling interaction through abstractions.
  2. Integration Example Updates:

    • Replaced the usage of CentrifugeClient and CentrifugeSubscription classes in the integration example with their corresponding Client and ClientSubscription protocols.
    • This demonstrates the ability to work through interfaces, simplifying testing and making implementation replacements easier.
  3. Public Initializers for Models:

    • All models (e.g., events, errors) used by the Centrifuge client now have public initializers.
    • This is necessary for creating instances of objects in client code, such as during testing.
  4. Property Optimization:

    • Properties that are immutable after initialization have been converted from var to let.
    • This improves code readability and safety. If changes are required, clients can create new structures using the public initializers.
  5. Demonstration of Protocol Usage:

    • Added an example demonstrating forced subscription to a channel when the application state changes (disconnect/connect). This shows that the protocol-based implementation is fully functional and compatible.

Suggestions for Future Improvements:

  • Consider fully transitioning to interaction through protocols instead of direct access to SDK classes.
  • Define more descriptive names for the protocols to better convey their purpose.
  • Reevaluate the approach to creating the Centrifuge client (e.g., using a factory instead of the current static constructor).

Notes:

All changes have been made with backward compatibility in mind. The existing logic of the client and subscriptions remains unchanged. This Pull Request simplifies Centrifuge integration into client projects and makes testing easier.

Demo project:

changes.demo.mp4

shalom aviv and others added 4 commits December 15, 2024 14:41
- CentrifugeClient implement Client protocol
- CentrifugeSubscription implement ClientSubscription protocol
- Client protocol adopted to use ClientSubscription instead of CentrifugeSubscription class
@shalom-aviv
Copy link
Contributor Author

Hi, @FZambia

What do you think about this pull request? ))

@FZambia
Copy link
Member

FZambia commented Jan 21, 2025

Hmm.. the complexity added here is sufficient and I need more time to think and more opinions on that.

Probably this may be addressed in alternative ways by mocking transport level and thus triggering required events, or by keeping protocols outside, by wrapping on app level or wrapper library level?

Is your main motivation to test your app or add more tests to centrifuge-swift? If it's application tests – then I guess it's already possible to do without introducing protocols, just constructors for types maybe. If testing centrifuge-swift itself – then I like protocol mock idea more.

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.

2 participants