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 a convention for exposing APIs to tests #72

Merged
merged 1 commit into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,21 @@ file_header:
# //
forbidden_pattern: // Created by

identifier_name:
type_name:
&no_length_checks # We disable the length checks, for the same reason we disable the rules of type "metrics".
min_length:
warning: 1
max_length:
warning: 10000

type_name: *no_length_checks

generic_type_name: *no_length_checks

identifier_name:
<<: *no_length_checks
allowed_symbols:
# Allow underscore; it can be handy for adding a prefix to another identifier (e.g. our testsOnly_<identifier> convention)
- _

# For compatibility with SwiftFormat
trailing_comma:
mandatory_comma: true
23 changes: 23 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,26 @@ 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.)

### Testing guidelines

#### Exposing test-only APIs

When writing unit tests, there are times that we need to access internal state of a type. To enable this, we might expose this state at an `internal` access level so that it can be used by the unit tests. However, we want to make it clear that this state is being exposed _purely_ for the purposes of testing that class, and that it should not be used by other components of the SDK.

So, when writing an API which has `internal` access level purely to enable it to be called by the tests, prefix this API’s name with `testOnly_`. For example:

```swift
private nonisolated let realtime: RealtimeClient

#if DEBUG
internal nonisolated var testsOnly_realtime: RealtimeClient {
realtime
}
#endif
```

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.
18 changes: 15 additions & 3 deletions Sources/AblyChat/Logging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,21 @@ extension InternalLogger {
}

internal final class DefaultInternalLogger: InternalLogger {
// Exposed for testing.
internal let logHandler: LogHandler
internal let logLevel: LogLevel
private let logHandler: LogHandler

#if DEBUG
internal var testsOnly_logHandler: LogHandler {
logHandler
}
#endif

private let logLevel: LogLevel

#if DEBUG
internal var testsOnly_logLevel: LogLevel {
logLevel
}
#endif
lawrence-forooghian marked this conversation as resolved.
Show resolved Hide resolved

internal init(logHandler: LogHandler?, logLevel: LogLevel?) {
self.logHandler = logHandler ?? DefaultLogHandler()
Expand Down
8 changes: 7 additions & 1 deletion Sources/AblyChat/Room.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,13 @@ internal actor DefaultRoom: Room {
internal nonisolated let options: RoomOptions

// Exposed for testing.
internal nonisolated let realtime: RealtimeClient
private nonisolated let realtime: RealtimeClient
lawrence-forooghian marked this conversation as resolved.
Show resolved Hide resolved

#if DEBUG
internal nonisolated var testsOnly_realtime: RealtimeClient {
realtime
}
#endif

private let _status: DefaultRoomStatus
private let logger: InternalLogger
Expand Down
10 changes: 8 additions & 2 deletions Sources/AblyChat/Rooms.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,14 @@ public protocol Rooms: AnyObject, Sendable {
}

internal actor DefaultRooms: Rooms {
/// Exposed so that we can test it.
internal nonisolated let realtime: RealtimeClient
private nonisolated let realtime: RealtimeClient

#if DEBUG
internal nonisolated var testsOnly_realtime: RealtimeClient {
realtime
}
#endif

internal nonisolated let clientOptions: ClientOptions

private let logger: InternalLogger
Expand Down
2 changes: 1 addition & 1 deletion Tests/AblyChatTests/DefaultChatClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ struct DefaultChatClientTests {
let rooms = client.rooms

let defaultRooms = try #require(rooms as? DefaultRooms)
#expect(defaultRooms.realtime === realtime)
#expect(defaultRooms.testsOnly_realtime === realtime)
#expect(defaultRooms.clientOptions.isEqualForTestPurposes(options))
}
}
4 changes: 2 additions & 2 deletions Tests/AblyChatTests/DefaultInternalLoggerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ struct DefaultInternalLoggerTests {
func defaults() {
let logger = DefaultInternalLogger(logHandler: nil, logLevel: nil)

#expect(logger.logHandler is DefaultLogHandler)
#expect(logger.logLevel == .error)
#expect(logger.testsOnly_logHandler is DefaultLogHandler)
#expect(logger.testsOnly_logLevel == .error)
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion Tests/AblyChatTests/DefaultRoomsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ struct DefaultRoomsTests {

// Then: It returns a DefaultRoom instance that uses the same Realtime instance, with the given ID and options
let defaultRoom = try #require(room as? DefaultRoom)
#expect(defaultRoom.realtime === realtime)
#expect(defaultRoom.testsOnly_realtime === realtime)
lawrence-forooghian marked this conversation as resolved.
Show resolved Hide resolved
#expect(defaultRoom.roomID == roomID)
#expect(defaultRoom.options == options)
}
Expand Down