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

Add an integration test for network fault coverage to Subscriber component #918

Closed
jaley opened this issue Jan 17, 2023 · 2 comments · Fixed by #924
Closed

Add an integration test for network fault coverage to Subscriber component #918

jaley opened this issue Jan 17, 2023 · 2 comments · Fixed by #924
Assignees

Comments

@jaley
Copy link
Contributor

jaley commented Jan 17, 2023

Many of the faults simulated in #865 and #886 apply equally to the Subscriber component as they do the Publisher. We went the approach of making NetworkConnectivityTest a publisher test as the scope for failures there was greater, and having internal access to the DefaultPublisher constructors greatly simplified injecting an AblyRealtime instance configured to connect to localhost without TLS. All of the fault simulation logic and proxy server implementations, however, are in android-test-common and so could be used from an integration test in the Subscriber, meaning we only need to write a test that uses it and exercises the Subscriber API appropriately during faults. Given that this is a separate API to the Publisher designed for different customer use cases, there's likely very little wastage in just implementing a whole new test that uses the existing proxy and fault code.

If we adopt a similar approach to that taken in the Publisher, the requirements are essentially:

  • Add a new integration test to the subscriber component
  • Make use of Junit Paramaterized test runner to inject the desired fault implementations
  • Create some helpers to instantiate DefaultSubscriber with an AblyRealtime instance that was constructed with ClientOptions provided by the proxy implementations (i.e. configured to connect locally without TLS)
  • Write integration-level tests that exercise Subscriber, activating the injected faults at opportune times and making assertions about the observed state of trackables and/or location updates before, after and during faults.

In terms of what use cases to exercise, an initial cursory look over the Subscriber interface suggests:

  • Activate fault before starting and stopping the subscriber - should not throw exceptions, as per design change agreed in Don't throw an exception on trackable add #879
  • Activate fault during tracking, publish a location update (can do this directly with Ably channel), ensure that the location update arrives when fault resolved by monitoring subscriber.locations and/or subscriber.rawLocations
  • Activate fault during tracking and ensure the expected Trackable state transitions happen by watching subscriber.trackableStates, and that this recovers when fault resolved
  • Activate fault during tracking and ensure subscriber.publisherPresent flag updates as expected, and updates when fault resolved

Perhaps lower priority:

  • ... same for publisher resolution updates through subscriber.resolutions?
  • Try updating resolutionPreferences during a fault, ensure that this takes affect after fault resolved.

Would be great to get a review on the above from @KacperKluka, @QuintinWillison and perhaps @paddybyers to ensure that the proposed tests are in line with (new) expected behaviours, and any comments on implementation strategy.

@sync-by-unito
Copy link

sync-by-unito bot commented Jan 17, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3256

@jaley
Copy link
Contributor Author

jaley commented Jan 17, 2023

Note to whoever picks this up - we're pretty sure ably/ably-java#474 will be impacting the subscriber too, so you'll want to pull in this update and confirm fixes when available.

@AndyTWF AndyTWF self-assigned this Jan 17, 2023
@AndyTWF AndyTWF linked a pull request Jan 23, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants