forked from ably/ably-chat-swift
-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ECO-4952] Start implementing room lifecycle spec #17
Closed
ruancomelli
wants to merge
1
commit into
base-sha/af413489e338411e4df93b7827ab89cc763ef5a8
from
head-sha/608f43e5e0e912cb5d90765c4492f9599527aa2b/2024-09-30T19-22-29/727751
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ To check formatting and code quality, run `swift run BuildTool lint`. Run with ` | |
- We should aim to make it easy for consumers of the SDK to be able to mock out the SDK in the tests for their own code. A couple of things that will aid with this: | ||
- Describe the SDK’s functionality via protocols (when doing so would still be sufficiently idiomatic to Swift). | ||
- When defining a `struct` that is emitted by the public API of the library, make sure to define a public memberwise initializer so that users can create one to be emitted by their mocks. (There is no way to make Swift’s autogenerated memberwise initializer public, so you will need to write one yourself. In Xcode, you can do this by clicking at the start of the type declaration and doing Editor → Refactor → Generate Memberwise Initializer.) | ||
- When writing code that implements behaviour specified by the Chat SDK features spec, add a comment that references the identifier of the relevant spec item. | ||
|
||
### Testing guidelines | ||
|
||
|
@@ -51,3 +52,49 @@ A couple of notes: | |
|
||
- Using a naming convention will allow us to verify that APIs marked `testsOnly` are not being used inside the SDK; we’ll do this in #70. | ||
- I believe that we should be able to eliminate the boilerplate of re-exposing a `private` member as a `testsOnly` member (as exemplified above) using a macro (called e.g. `@ExposedToTests`), but my level of experience with macros is insufficient to be confident about being able to do this quickly, so have deferred it to #71. | ||
|
||
#### Attributing tests to a spec point | ||
|
||
When writing a test that relates to a spec point from the Chat SDK features spec, add a comment that contains one of the following tags: | ||
|
||
- `@spec <spec-item-id>` — The test case directly tests all the functionality documented in the spec item. | ||
- `@specOneOf(m/n) <spec-item-id>` — The test case is the m<sup>th</sup> of n test cases which, together, test all the functionality documented in the spec item. | ||
- `@specPartial <spec-item-id>` — The test case tests some, but not all, of the functionality documented in the spec item. This is different to `@specOneOf` in that it implies that the test suite does not fully test this spec item. | ||
|
||
The `<spec-item-id>` parameter should be a spec item identifier such as `CHA-RL3g`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (documentation): Standardize terminology: 'spec point' vs 'spec item' The document uses both 'spec point' and 'spec item'. Consider standardizing on one term for consistency throughout the document. |
||
|
||
Each of the above tags can optionally be followed by a hyphen and an explanation of how the test relates to the given spec item. | ||
|
||
Examples: | ||
|
||
```swift | ||
// @spec CHA-EX3f | ||
func test1 { … } | ||
``` | ||
|
||
```swift | ||
// @specOneOf(1/2) CHA-EX2h — Tests the case where the room is FAILED | ||
func test2 { … } | ||
|
||
// @specOneOf(2/2) CHA-EX2h — Tests the case where the room is SUSPENDED | ||
func test3 { … } | ||
``` | ||
|
||
```swift | ||
// @specPartial CHA-EX1h4 - Tests that we retry, but not the retry attempt limit because we’ve not implemented it yet | ||
func test4 { … } | ||
``` | ||
|
||
In [#46](https://github.com/ably-labs/ably-chat-swift/issues/46), we’ll write a script that uses these tags to generate a report about how much of the feature spec we’ve implemented. | ||
|
||
#### Marking a spec point as untested | ||
|
||
In addition to the above, you can add the following as a comment anywhere in the test suite: | ||
|
||
- `@specUntested <spec-item-id> - <explanation>` — This indicates that the SDK implements the given spec point, but that there are no automated tests for it. This should be used sparingly; only use it when there is no way to test a spec point. It must be accompanied by an explanation of why this spec point is not tested. | ||
|
||
Example: | ||
|
||
```swift | ||
// @specUntested CHA-EX2b - I was unable to find a way to test this spec point in an environment in which concurrency is being used; there is no obvious moment at which to stop observing the emitted state changes in order to be sure that FAILED has not been emitted twice. | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/// The features offered by a chat room. | ||
internal enum RoomFeature { | ||
case messages | ||
case presence | ||
case reactions | ||
case occupancy | ||
case typing | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (documentation): Replace Markdown superscript syntax with plain text
The superscript syntax 'th' won't render correctly in plain text. Consider using 'mth' instead.