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

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Sep 30, 2024

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

    • Introduced a structured logging mechanism with enhanced logging capabilities in the Chat SDK.
    • Added a new section in the contribution guidelines for exposing test-only APIs.
  • Bug Fixes

    • Adjusted test cases to access properties using new test-only accessors for better encapsulation.
  • Documentation

    • Updated CONTRIBUTING.md to include guidelines on exposing internal state for testing.
  • Refactor

    • Enhanced encapsulation by restricting access to certain properties within the SDK.

Copy link

coderabbitai bot commented Sep 30, 2024

Walkthrough

The changes involve updates to the .swiftlint.yml configuration for stricter linting rules, enhancements to logging mechanisms in the Chat SDK, and modifications to the Room and Rooms classes for better encapsulation and testing access. The CONTRIBUTING.md file now includes guidance on exposing test-only APIs. Tests have been adjusted to reflect these changes, ensuring that internal states are accessed appropriately during testing.

Changes

File Change Summary
.swiftlint.yml Updated to exclude .build, set strict mode to true, disabled several metrics-related rules, added various opt-in rules, and modified file_header, identifier_name, and trailing_comma rules.
CONTRIBUTING.md Added a section on exposing test-only APIs with examples and guidelines for naming conventions.
Sources/AblyChat/Logging.swift Introduced LogContext type alias, enhanced InternalLogger protocol, modified DefaultInternalLogger for better encapsulation, and updated logging methods in DefaultLogHandler.
Sources/AblyChat/Room.swift Changed realtime property from internal to private, added testsOnly_realtime for debug builds, and maintained existing unimplemented properties.
Sources/AblyChat/Rooms.swift Updated realtime property to private, added testsOnly_realtime, and maintained method signatures while modifying implementations for error handling.
Tests/AblyChatTests/DefaultChatClientTests.swift Adjusted tests to access testsOnly_realtime instead of realtime for testing purposes.
Tests/AblyChatTests/DefaultInternalLoggerTests.swift Updated assertions to check testsOnly_logHandler and testsOnly_logLevel for testing purposes.
Tests/AblyChatTests/DefaultRoomsTests.swift Modified tests to access testsOnly_realtime in assertions while keeping other tests unchanged.

Possibly related PRs

Suggested reviewers

  • maratal
  • umair-ably

Poem

🐇 In the code, we hop and play,
With linting rules to guide our way.
Logging now has clearer sights,
Test-only paths bring joyful nights.
Encapsulated, neat, and bright,
Our code's a garden, pure delight! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6db65a3 and af41348.

📒 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)
🚧 Files skipped from review as they are similar to previous changes (8)
  • .swiftlint.yml
  • CONTRIBUTING.md
  • Sources/AblyChat/Logging.swift
  • Sources/AblyChat/Room.swift
  • Sources/AblyChat/Rooms.swift
  • Tests/AblyChatTests/DefaultChatClientTests.swift
  • Tests/AblyChatTests/DefaultInternalLoggerTests.swift
  • Tests/AblyChatTests/DefaultRoomsTests.swift

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.realtime to defaultRooms.testsOnly_realtime is a good implementation of the PR's objective to establish a convention for exposing APIs specifically for testing purposes. The testsOnly_ 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:

  1. Ensure this convention is applied consistently across the entire codebase.
  2. Update or create documentation explaining this new convention to other developers.
  3. 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 convention

The changes in this file successfully implement the new convention for exposing APIs specifically for testing purposes. The use of the testsOnly_ prefix in defaultRooms.testsOnly_realtime clearly 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:

  1. Ensure this naming pattern is applied consistently across all test files in the project.
  2. Consider updating the project's coding guidelines or README to document this new convention.
  3. 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 suggestion

The addition of testsOnly_realtime is 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 in get method with a suggestion for documentation

The changes to the get method 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 the release method

The release method 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 release method? 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 the log() test method

While this test method remains unchanged, it might be beneficial to apply the new testsOnly_ convention here as well. Currently, the test is directly accessing the logHandler, which could be inconsistent with the approach used in the defaults() 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 the DefaultInternalLogger class. 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 consistency

Similar to the log() test method, this test directly accesses the logHandler. To maintain consistency across all test methods and fully implement the new convention, consider updating this test to use a testsOnly_ prefixed property for accessing the log handler.

If a testsOnly_ prefixed property for accessing the log handler is available or can be added to the DefaultInternalLogger class, 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 convention

The 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:

  1. Consider applying the testsOnly_ prefix convention consistently across all test methods in this file.
  2. Ensure that the DefaultInternalLogger class has appropriate testsOnly_ prefixed properties for all necessary testing access.
  3. 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 get method of DefaultRooms. However, consider the following suggestions to further improve the test suite:

  1. Add test cases for edge cases, such as empty room IDs or null options.
  2. Consider testing the behavior when the MockRealtime.create() method fails or returns unexpected results.
  3. Enhance the @spec comments to provide more context about what each specification is testing.
  4. Add a test case to verify the behavior when calling get with a previously used room ID but with matching options (to contrast with the get_throwsErrorWhenOptionsDoNotMatch test).

Here's an example of how you could enhance the @spec comments:

// @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:

  1. For the mentions of Verify that testOnly APIs aren’t being used inside the codebase #70 and Try replacing some things with macros #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.

  2. 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.

  3. 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 of testOnly_, 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 todo rule 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_header rule 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:

  1. 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
  2. Create custom rules for specific naming patterns that require exceptions to the general rules.
  3. 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:

  1. Implementing more lenient but still reasonable length limits:
    identifier_name:
      min_length:
        warning: 2
      max_length:
        warning: 60
      excluded:
        - id
        - URL
        - i
        - j
  2. 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: warning

This approach maintains flexibility while still providing some guardrails for consistent naming.

Sources/AblyChat/Room.swift (1)

27-31: Approved: Good implementation of test-only API

The addition of testsOnly_realtime property aligns well with the PR objectives. The use of #if DEBUG and 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

📥 Commits

Files that changed from the base of the PR and between b35b114 and 78fd1be.

📒 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 of realtime property

Changing the visibility of realtime from internal to private is a good practice. It enhances encapsulation by restricting access to this property only within the DefaultRooms actor. 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 suggestions

Overall, 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 get method.

Key points:

  1. The realtime property is now properly encapsulated.
  2. The testsOnly_realtime property provides a clear, debug-only API for tests.
  3. The get method 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 release method 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_logHandler and testsOnly_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 .build directory 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:

  1. Appropriate exclusions and strict mode activation.
  2. Thoughtful disabling of certain rules with clear justifications.
  3. Comprehensive opt-in rules covering performance, style, and idiomatic Swift usage.
  4. Removal of boilerplate comments and allowance for test-specific naming conventions.
  5. Flexible naming rules that support the project's needs.
  6. 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 objectives

The modifications to the realtime property and the addition of testsOnly_realtime effectively 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 realtime private 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:

  1. Update or remove the outdated "Exposed for testing" comment.
  2. Add documentation for the usage of testsOnly_realtime in tests.

These minor adjustments will further enhance the clarity and usability of the implemented convention.

Tests/AblyChatTests/DefaultRoomsTests.swift Show resolved Hide resolved
Sources/AblyChat/Room.swift Show resolved Hide resolved
Sources/AblyChat/Logging.swift Show resolved Hide resolved
@lawrence-forooghian lawrence-forooghian force-pushed the convention-for-test-only-apis branch from 78fd1be to 6db65a3 Compare September 30, 2024 15:00
@lawrence-forooghian lawrence-forooghian force-pushed the convention-for-test-only-apis branch 3 times, most recently from 5ab7b44 to 9fa91c6 Compare September 30, 2024 15:23
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.
Copy link
Collaborator

@umair-ably umair-ably left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

big fan of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants