From 9fa91c68fea793b9a3589d5b9b2e3e5ddc723310 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Sep 2024 15:04:14 -0300 Subject: [PATCH] Add a convention for exposing APIs to tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I had noticed that I was writing lots of comments like "Exposed for testing" and thought it would be useful to do something more consistent and lintable. I’ve gated the code for re-exposing APIs behind a #if DEBUG, to avoid bloating release builds. Now that there’s a divergence between the Debug and Release configuration, we should update CI to also build in the Release configuration; have created #68 for this. --- .swiftlint.yml | 10 ++++++--- CONTRIBUTING.md | 21 +++++++++++++++++++ Sources/AblyChat/Logging.swift | 18 +++++++++++++--- Sources/AblyChat/Room.swift | 8 ++++++- Sources/AblyChat/Rooms.swift | 10 +++++++-- .../DefaultChatClientTests.swift | 2 +- .../DefaultInternalLoggerTests.swift | 4 ++-- Tests/AblyChatTests/DefaultRoomsTests.swift | 2 +- 8 files changed, 62 insertions(+), 13 deletions(-) diff --git a/.swiftlint.yml b/.swiftlint.yml index f27ca7f..e50be7c 100644 --- a/.swiftlint.yml +++ b/.swiftlint.yml @@ -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_ convention) + - _ + # For compatibility with SwiftFormat trailing_comma: mandatory_comma: true diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1771ced..3f9ed41 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,3 +28,24 @@ 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.) + +### 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. diff --git a/Sources/AblyChat/Logging.swift b/Sources/AblyChat/Logging.swift index fe95943..dbfd41e 100644 --- a/Sources/AblyChat/Logging.swift +++ b/Sources/AblyChat/Logging.swift @@ -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 internal init(logHandler: LogHandler?, logLevel: LogLevel?) { self.logHandler = logHandler ?? DefaultLogHandler() diff --git a/Sources/AblyChat/Room.swift b/Sources/AblyChat/Room.swift index e720931..e4660c8 100644 --- a/Sources/AblyChat/Room.swift +++ b/Sources/AblyChat/Room.swift @@ -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 + + #if DEBUG + internal nonisolated var testsOnly_realtime: RealtimeClient { + realtime + } + #endif private let _status: DefaultRoomStatus private let logger: InternalLogger diff --git a/Sources/AblyChat/Rooms.swift b/Sources/AblyChat/Rooms.swift index 12fd6ef..87f1d7c 100644 --- a/Sources/AblyChat/Rooms.swift +++ b/Sources/AblyChat/Rooms.swift @@ -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 diff --git a/Tests/AblyChatTests/DefaultChatClientTests.swift b/Tests/AblyChatTests/DefaultChatClientTests.swift index 1f207ae..569322c 100644 --- a/Tests/AblyChatTests/DefaultChatClientTests.swift +++ b/Tests/AblyChatTests/DefaultChatClientTests.swift @@ -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)) } } diff --git a/Tests/AblyChatTests/DefaultInternalLoggerTests.swift b/Tests/AblyChatTests/DefaultInternalLoggerTests.swift index a40bf22..9e8f80e 100644 --- a/Tests/AblyChatTests/DefaultInternalLoggerTests.swift +++ b/Tests/AblyChatTests/DefaultInternalLoggerTests.swift @@ -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 diff --git a/Tests/AblyChatTests/DefaultRoomsTests.swift b/Tests/AblyChatTests/DefaultRoomsTests.swift index e4e3bd9..adf9faf 100644 --- a/Tests/AblyChatTests/DefaultRoomsTests.swift +++ b/Tests/AblyChatTests/DefaultRoomsTests.swift @@ -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) #expect(defaultRoom.roomID == roomID) #expect(defaultRoom.options == options) }