Add a convention for exposing APIs to tests #72
Conversation
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
Tests/AblyChatTests/DefaultChatClientTests.swift (2)
26-26: Approve the new convention for exposing test-only APIs.The change from
defaultRooms.realtimetodefaultRooms.testsOnly_realtimeis a good implementation of the PR's objective to establish a convention for exposing APIs specifically for testing purposes. ThetestsOnly_prefix clearly indicates that this property is intended for use in tests only, which improves code clarity and maintainability.This change aligns well with the goal of creating a consistent and lintable convention. It will make it easier for developers to identify which APIs are exposed purely for testing, potentially reducing misuse in production code.
Consider the following to further improve this approach:
- Ensure this convention is applied consistently across the entire codebase.
- Update or create documentation explaining this new convention to other developers.
- If possible, implement a linter rule to enforce this naming convention for test-only APIs.
Line range hint
1-28: Overall approval of the new test-only API conventionThe changes in this file successfully implement the new convention for exposing APIs specifically for testing purposes. The use of the
testsOnly_prefix indefaultRooms.testsOnly_realtimeclearly distinguishes APIs intended for testing from those used in production code.This change aligns perfectly with the PR's objective of establishing a consistent and lintable convention for exposing APIs for testing purposes. It enhances code clarity and maintainability by making it explicit which parts of the API are meant for testing.
To further solidify this new convention:
- Ensure this naming pattern is applied consistently across all test files in the project.
- Consider updating the project's coding guidelines or README to document this new convention.
- If not already planned, consider implementing a custom linter rule to enforce this naming convention for test-only APIs across the entire codebase.
Sources/AblyChat/Rooms.swift (3)
12-16: Approved: Well-implemented test-only API with a minor suggestionThe addition of
testsOnly_realtimeis an excellent implementation of the PR's objective. It provides a clear, test-specific API that's only available in debug builds, maintaining encapsulation in production code.Consider using an underscore prefix for the property name to align with Swift's naming conventions for test-only APIs:
- internal nonisolated var testsOnly_realtime: RealtimeClient { + internal nonisolated var _testsOnly_realtime: RealtimeClient {This change would make it even clearer that this is not part of the public API.
Line range hint
31-45: Approved: Improved robustness ingetmethod with a suggestion for documentationThe changes to the
getmethod enhance its robustness by ensuring consistency in room options. The error handling for inconsistent options is a good practice, and the implementation correctly handles different scenarios.Consider adding a brief comment explaining the purpose of the consistency check and the meaning of the "CHA-RC1b" comment. This would improve code readability and maintainability. For example:
// Ensure consistency of room options (CHA-RC1b requirement) if let existingRoom = rooms[roomID] { // ... (rest of the code) }
Line range hint
48-48: Implement or track thereleasemethodThe
releasemethod is currently unimplemented. While this is not directly related to the PR's main objective, it's important to ensure that all protocol methods are properly implemented.Would you like me to create a GitHub issue to track the implementation of the
releasemethod? This will help ensure it's not overlooked in future development.Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3)
Line range hint
13-31: Consider applying the test-only API convention to thelog()test methodWhile this test method remains unchanged, it might be beneficial to apply the new
testsOnly_convention here as well. Currently, the test is directly accessing thelogHandler, which could be inconsistent with the approach used in thedefaults()test.Consider updating the test to use a
testsOnly_prefixed property for accessing the log handler, if such a property exists or can be added to theDefaultInternalLoggerclass. This would ensure consistency across all test methods and reinforce the new convention.
Line range hint
33-52: Consider applying the test-only API convention to this test method for consistencySimilar to the
log()test method, this test directly accesses thelogHandler. To maintain consistency across all test methods and fully implement the new convention, consider updating this test to use atestsOnly_prefixed property for accessing the log handler.If a
testsOnly_prefixed property for accessing the log handler is available or can be added to theDefaultInternalLoggerclass, update this test method to use it. This would ensure a uniform approach across all test methods and strengthen the implementation of the new convention.
Line range hint
1-52: Summary: Good progress on implementing the test-only API conventionThe changes in this file successfully introduce the
testsOnly_prefix convention for exposing APIs specifically for testing purposes. This aligns well with the PR objectives and improves the separation between public and test-only APIs.To further enhance the implementation:
- Consider applying the
testsOnly_prefix convention consistently across all test methods in this file.- Ensure that the
DefaultInternalLoggerclass has appropriatetestsOnly_prefixed properties for all necessary testing access.- Review other test files in the project to apply this convention broadly for maximum benefit.
These steps will help solidify the new convention and improve overall code clarity and maintainability.
Tests/AblyChatTests/DefaultRoomsTests.swift (1)
Line range hint
1-68: Consider enhancing test coverage and documentation.The existing tests provide good coverage for the
getmethod ofDefaultRooms. However, consider the following suggestions to further improve the test suite:
- Add test cases for edge cases, such as empty room IDs or null options.
- Consider testing the behavior when the
MockRealtime.create()method fails or returns unexpected results.- Enhance the
@speccomments to provide more context about what each specification is testing.- Add a test case to verify the behavior when calling
getwith a previously used room ID but with matching options (to contrast with theget_throwsErrorWhenOptionsDoNotMatchtest).Here's an example of how you could enhance the
@speccomments:// @spec CHA-RC1a: Verify that get() returns a room with the correct ID, options, and realtime instance @Test func get_returnsRoomWithGivenID() async throws { // ... existing test code ... } // @spec CHA-RC1b: Ensure that get() returns the same room object when called multiple times with the same ID and options @Test func get_returnsExistingRoomWithGivenID() async throws { // ... existing test code ... } // @spec CHA-RC1c: Confirm that get() throws an error when called with the same ID but different options @Test func get_throwsErrorWhenOptionsDoNotMatch() async throws { // ... existing test code ... }These enhancements would improve the overall quality and maintainability of the test suite.
CONTRIBUTING.md (1)
32-51: Great addition of guidelines for exposing test-only APIs!This new section aligns well with the PR objectives, providing a clear and consistent convention for exposing APIs specifically for testing purposes. The rationale, convention, and example are well-structured and informative.
Consider the following suggestions to further improve this section:
For the mentions of Verify that
testOnlyAPIs aren’t being used inside the codebase #70 and Write a macro for the pattern of re-exposing APIs for tests #71, it would be helpful to provide a brief description of what these issues entail, as not all contributors may have context on these issue numbers.It might be beneficial to briefly explain why the
testOnly_prefix was chosen over other potential solutions. This could help contributors understand the reasoning behind the convention.In the example (line 42), there's an inconsistency:
- internal nonisolated var testsOnly_realtime: RealtimeClient { + internal nonisolated var testOnly_realtime: RealtimeClient {The example uses
testsOnly_instead oftestOnly_, which doesn't match the stated convention. Please update for consistency..swiftlint.yml (4)
Line range hint
6-18: LGTM: Justified rule disabling with room for improvement.The disabling of metrics-related rules is well-justified, as arbitrary defaults may not suit every project. The explanation for disabling the
todorule is also reasonable.Consider implementing a custom rule or using a regex pattern to enforce a specific format for TODOs, such as requiring a GitHub issue reference. This could help maintain consistency in TODO comments while still allowing their use.
Example:
custom_rules: todo_format: name: "TODO Format" regex: "(TODO|FIXME)(?!:? #[0-9]+)" message: "TODO/FIXME comment should include a GitHub issue number (e.g., TODO: #123)" severity: warning
Line range hint
78-90: LGTM: Effective removal of boilerplate comments.The
file_headerrule effectively prevents Xcode-generated boilerplate comments, improving code cleanliness and readability. This aligns well with the PR's goal of establishing consistent conventions.Consider adding a required pattern for file headers that includes essential information like copyright notice or a brief description of the file's purpose. This could further enhance consistency and provide valuable context for each file.
Example:
file_header: required_pattern: | \/\/ \/\/ .*?\.swift \/\/ YourProjectName \/\/ \/\/ Copyright © \d{4} YourCompany\. All rights reserved\. \/\/
92-101: Approved with suggestions: Flexible naming approach.Disabling length checks for type names and generic type names allows for flexibility, which can be beneficial in certain scenarios. However, this approach might lead to inconsistencies in naming conventions across the project.
Consider implementing a more balanced approach:
- Instead of completely disabling length checks, set more lenient but still reasonable limits. For example:
type_name: min_length: warning: 3 max_length: warning: 50 excluded: - ID - URL - URI- Create custom rules for specific naming patterns that require exceptions to the general rules.
- Document the reasoning behind these choices in a code style guide to ensure team-wide understanding and consistency.
104-108: Approved with suggestions: Identifier naming convention.Allowing underscores in identifier names is a good decision, especially for the convention of exposing APIs to tests (e.g.,
testsOnly_<identifier>). This aligns well with the PR objectives.However, completely disabling length checks for identifiers might lead to inconsistencies. Consider:
- Implementing more lenient but still reasonable length limits:
identifier_name: min_length: warning: 2 max_length: warning: 60 excluded: - id - URL - i - j- Creating a custom rule for the
testsOnly_prefix:custom_rules: tests_only_prefix: name: "Tests Only Prefix" regex: "testsOnly_[a-z]" message: "Identifiers exposed for testing should use the testsOnly_ prefix followed by a lowercase letter" severity: warningThis approach maintains flexibility while still providing some guardrails for consistent naming.
Sources/AblyChat/Room.swift (1)
27-31: Approved: Good implementation of test-only APIThe addition of
testsOnly_realtimeproperty aligns well with the PR objectives. The use of#if DEBUGand the naming convention clearly indicate its intended use for testing purposes.Consider adding a documentation comment explaining how to use this property in tests. For example:
#if DEBUG /// Provides access to the underlying `RealtimeClient` instance for testing purposes. /// This property is only available in debug builds and should not be used in production code. /// /// Usage example: /// ``` /// let room = DefaultRoom(...) /// let realtime = room.testsOnly_realtime /// // Use realtime in your tests /// ``` internal nonisolated var testsOnly_realtime: RealtimeClient { realtime } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .swiftlint.yml (1 hunks)
- CONTRIBUTING.md (1 hunks)
- Sources/AblyChat/Logging.swift (1 hunks)
- Sources/AblyChat/Room.swift (1 hunks)
- Sources/AblyChat/Rooms.swift (1 hunks)
- Tests/AblyChatTests/DefaultChatClientTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (1 hunks)
🧰 Additional context used
📓 Learnings (1)
Tests/AblyChatTests/DefaultChatClientTests.swift (1)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#61 File: Tests/AblyChatTests/DefaultRoomTests.swift:33-34 Timestamp: 2024-09-19T18:44:56.819Z Learning: In Swift Testing, using `#expect` with `await` may result in less informative failure messages when debugging asynchronous tests.
🔇 Additional comments (8)
Sources/AblyChat/Rooms.swift (2)
10-10: Approved: Improved encapsulation ofrealtimepropertyChanging the visibility of
realtimefrominternaltoprivateis a good practice. It enhances encapsulation by restricting access to this property only within theDefaultRoomsactor. This change aligns well with the PR's objective of establishing a convention for exposing APIs to tests.
Line range hint
1-48: Summary: Well-implemented changes with minor suggestionsOverall, the changes in this file effectively implement the PR's objective of establishing a convention for exposing APIs to tests. The modifications improve encapsulation, provide a clear test-only API, and enhance the robustness of the
getmethod.Key points:
- The
realtimeproperty is now properly encapsulated.- The
testsOnly_realtimeproperty provides a clear, debug-only API for tests.- The
getmethod now ensures consistency in room options.Consider the minor suggestions provided in the previous comments to further improve the code. Also, don't forget to implement the
releasemethod in future development.Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1)
9-10: Excellent implementation of the test-only API convention!The changes to use
testsOnly_prefixed properties (testsOnly_logHandlerandtestsOnly_logLevel) align perfectly with the PR objectives. This convention clearly distinguishes between the public API and test-specific functionality, reducing the risk of accidentally using test-only APIs in production code. It also enhances code readability by explicitly marking properties intended for testing purposes..swiftlint.yml (4)
Line range hint
1-4: LGTM: Appropriate exclusion and strict mode configuration.The exclusion of the
.builddirectory is a good practice to avoid linting generated files. Enabling strict mode aligns well with the PR objective of establishing a consistent convention for the codebase.
Line range hint
20-76: Excellent selection of opt-in rules aligning with PR objectives.The comprehensive list of opt-in rules, covering performance, style, idiomatic usage, and additional linting, strongly supports the PR's goal of establishing a consistent and lintable convention. The clear categorization and comments provide valuable context for each rule's inclusion.
This approach will significantly enhance code quality and maintainability across the project.
110-112: LGTM: Consistent trailing comma usage.Setting mandatory trailing commas is an excellent choice for maintaining consistency and compatibility with SwiftFormat. This rule will lead to cleaner diffs when modifying lists or dictionaries and aligns well with modern Swift coding practices.
Line range hint
1-112: Summary: Excellent progress towards consistent and lintable conventions.This SwiftLint configuration file demonstrates a strong commitment to code quality and consistency, aligning perfectly with the PR's objective of establishing a convention for exposing APIs to tests. The changes include:
- Appropriate exclusions and strict mode activation.
- Thoughtful disabling of certain rules with clear justifications.
- Comprehensive opt-in rules covering performance, style, and idiomatic Swift usage.
- Removal of boilerplate comments and allowance for test-specific naming conventions.
- Flexible naming rules that support the project's needs.
- Consistency in trailing comma usage for better compatibility with SwiftFormat.
These changes collectively contribute to a more maintainable and consistent codebase. The suggestions provided in the review comments, if implemented, would further refine this approach, striking a balance between flexibility and standardization.
Great job on moving towards a more structured and lintable codebase!
Sources/AblyChat/Room.swift (1)
25-31: Summary: Changes align well with PR objectivesThe modifications to the
realtimeproperty and the addition oftestsOnly_realtimeeffectively implement a convention for exposing APIs for testing purposes. This aligns perfectly with the PR objectives of establishing a consistent and lintable convention for test-specific APIs.The changes enhance encapsulation by making
realtimeprivate while still providing a clear, debug-only access point for tests. This approach improves code maintainability and clarity regarding which APIs are intended for testing.Consider addressing the suggested improvements:
- Update or remove the outdated "Exposed for testing" comment.
- Add documentation for the usage of
testsOnly_realtimein tests.These minor adjustments will further enhance the clarity and usability of the implemented convention.
| 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using @testable import to expose properties for testing
Currently, logHandler and logLevel are private and conditionally exposed for testing using testsOnly_ prefixed properties within #if DEBUG directives. This approach increases code complexity and can be simplified. By making logHandler and logLevel internal instead of private, you can use @testable import in your test target to access these properties directly. This avoids the need for conditional compilation and keeps the production code cleaner.
There was a problem hiding this comment.
Did you see the justification that I wrote in CONTRIBUTING.md?
There was a problem hiding this comment.
@lawrence-forooghian Yes, I see the justification you provided in CONTRIBUTING.md for using the testsOnly_ prefix within #if DEBUG directives to expose properties for testing. Thank you for the clarification.
( ・ω・)？
✏️ Learnings added
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#72
File: Sources/AblyChat/Logging.swift:47-61
Timestamp: 2024-09-30T14:58:50.693Z
Learning: In this project, test-only APIs should be exposed using the `testsOnly_` prefix within `#if DEBUG` directives, as per the guidelines in `CONTRIBUTING.md`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Do you agree with the justification, or do you think your originally-suggested approach is better? If so, why?
There was a problem hiding this comment.
@lawrence-forooghian, thank you for pointing me to your justification in CONTRIBUTING.md. After reviewing it, I understand that using the testsOnly_ prefix within #if DEBUG directives makes it explicit which properties are intended solely for testing. This approach ensures that test-only APIs are excluded from release builds, enhancing code safety by preventing accidental usage in production.
However, I still believe that using @testable import alongside internal access levels is a more idiomatic approach in Swift. Here are some reasons why:
- Simplicity and Cleanliness: By marking properties as
internal, you avoid additional code complexity from conditional compilation and prefixed property names. This keeps the production codebase cleaner and more maintainable. - Maintaining Access Control:
@testable importallows test targets to accessinternalmembers without exposing them publicly. This leverages Swift's access control mechanisms without introducing new conventions. - Consistency Across Builds: Relying on conditional compilation can sometimes lead to discrepancies between debug and release builds. Using
internalaccess levels ensures that the code behaves consistently, reducing the risk of unexpected issues.
That said, your approach does offer explicitness and an extra layer of safety by clearly denoting test-only code. If your project prioritizes these aspects and prefers strict separation between test and production code, your method is valid and justifiable.
Ultimately, both approaches have their merits, and the best choice depends on your project's specific needs and team preferences.
There was a problem hiding this comment.
This approach ensures that test-only APIs are excluded from release builds, enhancing code safety by preventing accidental usage in production.
The fact that the testOnly APIs are wrapped in #DEBUG is not important; stopping test-only code from being used in production is a non-goal of this PR. It was just a minor optimisation.
However, I still believe that using @testable import alongside internal access levels is a more idiomatic approach in Swift.
How do you suggest that we make it clear that a given API with internal access level must not be used by other components of the SDK and that it's only intended for testing?
By marking properties as internal, you avoid additional code complexity from conditional compilation and prefixed property names.
Yes, true, but as I mentioned in the documentation, this boilerplate code can be eliminated through the use of macros (#71).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
No, I was asking you a question @coderabbitai
There was a problem hiding this comment.
@lawrence-forooghian, thank you for clarifying your approach. I apologize for the misunderstanding earlier. I now see that preventing test-only code from being used in production is not the primary goal.
Regarding your question about making it clear that an internal API is intended only for testing and should not be used by other components of the SDK, your method of using the testsOnly_ prefix within #if DEBUG directives effectively communicates this intent. This explicit naming convention helps developers immediately recognize the purpose of these APIs.
Additionally, your plan to eliminate boilerplate code through the use of macros (as mentioned in #71) will streamline the process further. Given this context, your approach aligns well with your project's goals and ensures clarity within the codebase.
Thank you for the detailed explanation.
There was a problem hiding this comment.
And, to address this point:
Relying on conditional compilation can sometimes lead to discrepancies between debug and release builds
We have #68 for this; have updated commit message to mention.
lawrence-forooghian
force-pushed
the
convention-for-test-only-apis
branch
from
78fd1be
to
6db65a3
Compare
lawrence-forooghian
force-pushed
the
convention-for-test-only-apis
branch
3 times, most recently
from
5ab7b44
to
9fa91c6
Compare
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.
lawrence-forooghian
force-pushed
the
convention-for-test-only-apis
branch
from
9fa91c6
to
af41348
Compare
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.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
CONTRIBUTING.mdto include guidelines on exposing internal state for testing.Refactor