Write unit tests for DefaultAbly
#869
Comments
|
➤ Automation for Jira commented: The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3202 |
lawrence-forooghian
added a commit
that referenced
this issue
Dec 28, 2022
The DefaultAbly class already has quite a bit of logic (e.g. retries) and I intend to add further logic to it. It would be good to be able to unit test this class. It sounds like we haven’t done this so far because we didn’t want to try and mock the AblyRealtime class. But I think we need to be able to unit test DefaultAbly (which I’ve created issue #869 for), so I’m putting the ably-java library behind a set of AblySDK* interfaces. This will allow us to provide mock implementations of the Ably client library when testing DefaultAbly. (I’ve taken this approach from the asset-tracking-swift codebase.) One thing worth noticing is the new use of the !! operator when extracting the `reason` from a connection that’s in a failed state. This looks like it’s introducing a new unsafe behaviour, but it’s actually just making explicit a previous assumption, namely that when a connection is in a failed state its `reason` will be non-null. The previous version of the code - in which the connection’s `reason` was of platform type ErrorInfo! – would equally have thrown a NullPointerException if this assumption were violated.
lawrence-forooghian
added a commit
that referenced
this issue
Dec 29, 2022
The DefaultAbly class already has quite a bit of logic (e.g. retries) and I intend to add further logic to it. It would be good to be able to unit test this class. It sounds like we haven’t done this so far because we didn’t want to try and mock the AblyRealtime class. But I think we need to be able to unit test DefaultAbly (which I’ve created issue #869 for), so I’m putting the ably-java library behind a set of AblySdk* interfaces. This will allow us to provide mock implementations of the Ably client library when testing DefaultAbly. (I’ve taken this approach from the asset-tracking-swift codebase.) One thing worth noticing is the new use of the !! operator when extracting the `reason` from a connection that’s in a failed state. This looks like it’s introducing a new unsafe behaviour, but it’s actually just making explicit a previous assumption, namely that when a connection is in a failed state its `reason` will be non-null. The previous version of the code - in which the connection’s `reason` was of platform type ErrorInfo! – would equally have thrown a NullPointerException if this assumption were violated.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 4, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its and implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 4, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 4, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 5, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 5, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 9, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 9, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 9, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 9, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 10, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 10, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 10, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 10, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 11, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 11, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 11, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 11, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 11, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
lawrence-forooghian
added a commit
that referenced
this issue
Jan 12, 2023
These document the current behaviour of this method. I wrote them having only seen the Ably interface (which DefaultAbly implements), and knowing the dependencies that DefaultAbly has. I intentionally did not look at the code whilst writing these tests because I wanted to limit myself to writing test cases based on scenarios that I could imagine might occur, instead of ones based on logic that I know to exist. Once I’ve written tests like this for the whole class, then I’d like to look at its implementation and add any further test cases for logic that wasn’t clear from the public interface. This is part of #869.
|
After #914 is merged, the remaining work for this issue is the following:
|
|
Removed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
DefaultAblyhas quite a bit of logic, such as error handling and behaviours that vary based on the channel’s state. Currently this class is not unit tested, except for the one example test added in #868 and a few forcloseadded in #874. I think we should add tests for it, in order to document some of the key aspects of its current behaviour, and to help give us confidence when modifying it in the future.The text was updated successfully, but these errors were encountered: