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

Networking connectivity integration test using local proxy #866

Merged
merged 49 commits into from
Jan 13, 2023

Conversation

jaley
Copy link
Contributor

@jaley jaley commented Dec 28, 2022

@cruickshankpg here's an empty test and a place to put the proxy implementation. Let's discuss the interface we're working to initial so we can both get started.

@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka December 28, 2022 11:36 Inactive
@jaley
Copy link
Contributor Author

jaley commented Dec 28, 2022

@cruickshankpg - this will now run a simple integration test, which publishes a bunch of location updates for a single Trackable, using an injected AblyRealtime instance. This should be enough for you to put a proxy in the middle and confirm that you can break the test by interfering with traffic? Can also disable TLS as required.

@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka December 28, 2022 14:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka December 28, 2022 15:12 Inactive
@jaley
Copy link
Contributor Author

jaley commented Jan 5, 2023

This branch now has failing tests, which I believe reproduce some of our open issues:

I've not yet implemented a Layer 7 proxy, which is needed for the rest of the high priority fault types, so I'll try to get that done next. Tomorrow is really busy for me, but I'll see what I can do.

It might be a good idea to try to get this reviewed and ready to merge before then, if anyone wants to use the tests we have so far as a reproduction of the issues above to validate fixes? (Assuming that turns them green...)

Word of warning -- these are slow tests when they're failing. Generally, the pass in 2-3 seconds each, but waiting for ably-java timeouts when things are going wrong takes a few minutes. We may be able to tweak some of the timeouts in ClientOptions to speed up testing, but sadly I think some important ones are not client-configurable, e.g. the time taken for a channel to move to suspended state.

@jaley jaley changed the title Barebones test and proxy interface for issue #865 Networking Connectivity integration test using local proxy#865 Jan 5, 2023
@jaley jaley changed the title Networking Connectivity integration test using local proxy#865 Networking Connectivity integration test using local proxy #865 Jan 5, 2023
@jaley jaley changed the title Networking Connectivity integration test using local proxy #865 Networking connectivity integration test using local proxy Jan 5, 2023
@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka January 6, 2023 17:35 Inactive
@QuintinWillison
Copy link
Contributor

At 1e96b3a, locally from Android Studio, I am seeing:

Screenshot 2023-01-11 at 17 49 07

With the following failures:

Test Failure
faultDuringTracking[TcpConnectionRefused] com.ably.tracking.ConnectionException: failed to connect to localhost/127.0.0.1 (port 13579): connect failed: ECONNREFUSED (Connection refused)
faultBeforeAddingTrackable[TcpConnectionRefused] java.lang.AssertionError: Expectation 'attempt to add active Trackable while fault active' did not result in success.
faultDuringTracking[TcpConnectionUnresponsive] com.ably.tracking.ConnectionException: Timeout was thrown when waiting for channel to attach
faultBeforeAddingTrackable[TcpConnectionUnresponsive] java.lang.AssertionError: Expectation 'attempt to add active Trackable while fault active' did not result in success.

@jaley Is this what you would expect at this stage with this pull request? If so, then I would like to annotate those tests as ignored for now to make this pull request green and mergeable to main branch.

@jaley
Copy link
Contributor Author

jaley commented Jan 11, 2023

Nice work, @QuintinWillison!

Yes, I can confirm that I get the exact same failures, even on the #886 branch, as I don't think any of the relevant fixes have landed in main yet. In case you want to add comments/etc as to why these tests are ignored:

(Of course, there's no guarantee that there won't be more issues masked by those...)

@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka January 12, 2023 10:32 Inactive
@QuintinWillison QuintinWillison marked this pull request as ready for review January 12, 2023 12:12
@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka January 12, 2023 16:45 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/866/dokka January 13, 2023 11:52 Inactive
Copy link
Contributor

@davyskiba davyskiba left a comment

Choose a reason for hiding this comment

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

I have a few kotlinish suggestions, but we can address them later

Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

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

I skimmed over proxy implementation and test helper classes details, however, since the tests run and work I think it's safe to say that it looks good 👍 Also left a few suggestions/questions but they shouldn't block this PR 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants