[ECO-4990] Switch to Swift Testing #61
Conversation
WalkthroughThe changes involve a comprehensive transition from the XCTest framework to a new testing framework across multiple test files. This includes restructuring test classes into structs, changing method definitions to use Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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: 2
Outside diff range and nitpick comments (3)
Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1)
50-50: Nitpick: Consistency in checking fornilConsider using
#requirefor assertingnilvalues for consistency with other tests.Apply this diff to improve consistency:
- #expect(logHandler.logArguments == nil) + #require(logHandler.logArguments == nil)Tests/AblyChatTests/DefaultRoomTests.swift (2)
30-30: Check clarity of failure messages in looped assertionsUsing
#expectinside a loop is effective, but if an assertion fails, it might be unclear which iteration caused the failure. Consider enhancing the assertion to include the channel name for better diagnostics.
92-92: Enhance failure messages in looped assertionsSimilar to previous loops, if an assertion fails inside the loop, identifying the specific channel can be challenging. Include additional context in the assertion to aid in debugging.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- Tests/AblyChatTests/DefaultChatClientTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (5 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (4 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/MessageSubscriptionTests.swift (2 hunks)
- Tests/AblyChatTests/SubscriptionTests.swift (1 hunks)
Additional context used
SwiftLint
Tests/AblyChatTests/DefaultRoomsTests.swift
[Error] 3-3: Imports should be sorted
(sorted_imports)
Additional comments not posted (35)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
8-15: Excellent refactoring of the error assertion function!The changes made to the
isChatErrorfunction significantly improve its usability and simplify the logic:
- Changing the return type to
Boolallows the function to be used in contexts where a simple Boolean check is sufficient, rather than throwing exceptions.- Using a guard statement to check the error type and returning
falseif it doesn't match eliminates the need for unwrapping and assertions, reducing the chances of exceptions being thrown.- Checking the error properties only if the error is of the expected type simplifies the logic and improves readability.
These changes align with the PR objectives of modernizing the testing approach and enhancing performance. Great job!
Tests/AblyChatTests/InternalLoggerTests.swift (5)
2-2: LGTM!The import statement change aligns with the PR objective of migrating from XCTest to Swift Testing.
4-4: LGTM!The change from class to struct aligns with the recommended way to define test suites in Swift Testing.
5-6: LGTM!The changes to the test method, including the
@Testattribute and the removal of thetestprefix, align with the conventions in Swift Testing and the stated PR objectives.
12-12: LGTM!The change from
XCTAssertNoThrowto#requirealigns with the assertion syntax in Swift Testing.
14-16: LGTM!The change from
XCTAssertEqualto#expectaligns with the assertion syntax in Swift Testing.Tests/AblyChatTests/SubscriptionTests.swift (2)
6-10: LGTM!The test has been successfully refactored to use the new Swift Testing framework while maintaining the original test logic. The
#expectassertion correctly verifies the emitted elements from the subscription.
13-22: LGTM!The test has been successfully refactored to use the new Swift Testing framework while maintaining the original test logic. The
#expectassertion correctly verifies the emitted elements from the subscription.Tests/AblyChatTests/DefaultChatClientTests.swift (4)
2-2: Looks good!The import of the
Testingframework aligns with the PR objective of transitioning to Swift Testing.
Line range hint
4-28: Great work on transitioning the tests to Swift Testing!The changes effectively migrate the test structure and assertions to align with Swift Testing conventions while preserving the core test logic. This is a significant step towards modernizing the testing approach.
25-25: Looks good!The use of
#requireto unwraproomsaligns with Swift Testing syntax while maintaining the assertion logic.
26-27: Great use of#expectfor assertions!The assertions effectively verify the
realtimeandclientOptionsproperties ofdefaultRoomsusing Swift Testing's#expectsyntax.Tests/AblyChatTests/DefaultRoomStatusTests.swift (3)
5-8: LGTM!The test logic is correct, and the changes align with the migration to the new testing framework.
11-14: LGTM!The test logic is correct, and the changes align with the migration to the new testing framework.
Line range hint
17-39: LGTM!The test logic is correct, and the changes align with the migration to the new testing framework. The use of
#requireenhances clarity in the test's intent.Tests/AblyChatTests/MessageSubscriptionTests.swift (3)
26-30: LGTM!The test logic is correct and aligns with the objectives of migrating to the new testing framework. The use of
@Testannotation and#expectassertion syntax is appropriate.
33-42: LGTM!The test logic is correct and aligns with the objectives of migrating to the new testing framework. The use of
@Testannotation and#expectassertion syntax is appropriate.
45-53: LGTM!The test logic is correct and aligns with the objectives of migrating to the new testing framework. The use of
@Testannotation,#requirefor unwrapping optionals, and#expectassertion syntax is appropriate.Tests/AblyChatTests/DefaultRoomsTests.swift (3)
Line range hint
7-22: LGTM!The test has been successfully updated to use the new testing framework syntax while preserving the original test logic. The assertions verify the expected behavior of the
getmethod.
Line range hint
26-40: LGTM!The test has been successfully updated to use the new testing framework syntax while preserving the original test logic. The assertion verifies that calling the
getmethod with the same room ID returns the same room object.
Line range hint
44-66: LGTM!The test has been successfully updated to use the new testing framework syntax while preserving the original test logic. The assertion verifies that calling the
getmethod with the same room ID but different options throws aninconsistentRoomOptionserror.Tests/AblyChatTests/DefaultInternalLoggerTests.swift (6)
2-2: Approved: Importing the Testing frameworkThe addition of
import Testingaligns with the migration to the new Swift Testing framework.
4-4: Approved: Transition to struct-based test suitesSwitching from a class to a struct for the test suite is appropriate with the new testing framework and reflects the updated testing conventions.
5-10: Approved: Test for default logger initializationThe
defaults()test correctly verifies thatDefaultInternalLoggerinitializes with the expected default log handler and log level.
Line range hint
13-30: Approved: Test for logging behavior at default log levelThe
log()test effectively checks that the logger logs messages appropriately when the log level meets the required severity, including interpolation of code location into the message.
27-27: Confirm necessity oftrywith#requireUsing
trywith#requiresuggests thatlogHandler.logArgumentsmight throw an error or be asynchronous. Ensure that this is intended and thatlogHandler.logArgumentsrequires error handling.
Line range hint
33-50: Approved: Test for logging suppression at lower log levelsThe
log_whenLogLevelArgumentIsLessSevereThanLogLevelProperty_itDoesNotLog()test correctly verifies that the logger suppresses logging when the message's log level is less severe than the logger's configured log level.Tests/AblyChatTests/DefaultRoomTests.swift (8)
3-3: Properly import the Testing frameworkThe addition of
import Testingcorrectly introduces the new testing framework needed for Swift Testing.
5-5: Verify the impact of changing from class to structChanging
DefaultRoomTestsfrom a class to a struct alters its reference semantics to value semantics. Ensure this change doesn't affect any inherited behaviors or shared states within your test suite.
6-6: Correct use of @test annotationThe
@Testannotation is properly applied to the test method, enabling it to be recognized by the Swift Testing framework.
37-37: @test annotation correctly addedThe test method is appropriately annotated with
@Test, ensuring it is executed by the testing framework.
65-65: Ensure correct error comparison in assertionThe assertion compares
roomAttachErrorandchannelAttachErrorusing identity (===). Verify that both errors are expected to be the same instance. If only the error information needs to match, consider comparing their properties instead.
68-68: Proper application of @test annotationThe
@Testannotation is correctly used, allowing the test method to be recognized by the framework.
99-99: @test annotation correctly usedThe test method is correctly annotated with
@Test, which is necessary for the Swift Testing framework.
127-127: Verify error identity assertionConfirm that using the identity operator (
===) is appropriate for comparingroomDetachErrorandchannelDetachError. If they are not guaranteed to be the same instance, consider comparing their error codes or messages instead.
lawrence-forooghian
force-pushed
the
55-switch-to-swift-testing
branch
2 times, most recently
from
86614e6
to
ca271d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
8-8: Consider renaming themaybeErrorparameter toerrorfor better readability.Since the function no longer force unwraps the optional error, the
maybeprefix in the parameter name is not necessary. Renaming it toerrorwould improve readability without losing clarity.Apply this diff to rename the parameter:
-func isChatError(_ maybeError: (any Error)?, withCode code: AblyChat.ErrorCode) -> Bool { +func isChatError(_ error: (any Error)?, withCode code: AblyChat.ErrorCode) -> Bool { guard let ablyError = maybeError as? ARTErrorInfo else { return false }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- Tests/AblyChatTests/DefaultChatClientTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (5 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (4 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/MessageSubscriptionTests.swift (2 hunks)
- Tests/AblyChatTests/SubscriptionTests.swift (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
- Tests/AblyChatTests/InternalLoggerTests.swift
- Tests/AblyChatTests/SubscriptionTests.swift
Additional context used
Learnings (3)
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.Tests/AblyChatTests/DefaultRoomTests.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.Tests/AblyChatTests/DefaultRoomsTests.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 not posted (28)
Tests/AblyChatTests/Helpers/Helpers.swift (2)
3-3: LGTM!The import statement is correct and necessary for using the new testing framework.
6-15: Verify the function signature change in the codebase.The new function implementation is correct and enhances usability by returning a Boolean value instead of throwing exceptions. However, ensure that all calls to
assertIsChatErrorhave been updated to use the newisChatErrorfunction and handle the Boolean return value appropriately.Run the following script to verify the function usage:
Verification successful
To ensure a comprehensive verification, we need to check for other test files that might be using the
isChatErrorfunction. Let's run an additional script to search for all test files and their contents.
It seems there was an issue with the 'rg' command in the previous script. Let's try a different approach using 'grep' to search for 'isChatError' usage in all test files.
Function signature change verified, consider expanding test coverage.
The verification process confirms that the
isChatErrorfunction has been correctly implemented and is being used appropriately in the test files. The oldassertIsChatErrorfunction has been completely removed, and the newisChatErrorfunction is being used with the correct syntax to handle its Boolean return value.However, it's worth noting that the function is only used in one test file (DefaultRoomsTests.swift). Consider the following suggestions:
- Review other test files to determine if additional test cases using
isChatErrorwould be beneficial for more comprehensive error handling coverage.- Ensure that all relevant error scenarios across different components of the AblyChatTests are adequately tested using this new function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `assertIsChatError` have been updated to use `isChatError`. # Test 1: Search for the old function usage. Expect: No occurrences. rg --type swift $'assertIsChatError' # Test 2: Search for the new function usage. Expect: Only occurrences with appropriate handling of the Boolean return value. rg --type swift -A 5 $'isChatError'Length of output: 784
Script:
#!/bin/bash # Description: Find all test files and check for isChatError usage # Find all test files echo "All test files:" fd -e swift -p 'Tests/' # Search for isChatError usage in all test files echo -e "\nisChatError usage in test files:" rg --type swift -p 'Tests/' 'isChatError'Length of output: 996
Script:
#!/bin/bash # Description: Find isChatError usage in all test files using grep echo "isChatError usage in test files:" grep -r "isChatError" Tests/Length of output: 357
Tests/AblyChatTests/DefaultChatClientTests.swift (2)
5-12: LGTM!The test migration looks good. The logic remains the same, and the assertion has been correctly updated to use
#expectfrom Swift Testing.
Line range hint
15-27: LGTM!The test migration looks good. The logic remains the same, and the assertions have been correctly updated to use
#expectand#requirefrom Swift Testing.Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3)
5-10: LGTM!The test logic remains the same, and the new
#expectsyntax is used correctly for assertions.
Line range hint
13-30: LGTM!The test logic remains the same, and the new
#requireand#expectsyntax is used correctly for assertions.
Line range hint
33-50: LGTM!The test logic remains the same, and the new
#expectsyntax is used correctly for the assertion.Tests/AblyChatTests/MessageSubscriptionTests.swift (3)
26-30: LGTM!The test logic is correct and aligns with the migration objectives. The use of
@Testannotation and#expectassertion syntax is consistent with the new testing framework.
33-42: LGTM!The test logic is correct and aligns with the migration objectives. The use of
@Testannotation and#expectassertion syntax is consistent with the new testing framework.
45-53: LGTM!The test logic is correct and aligns with the migration objectives. The use of
@Testannotation,#requirefor unwrapping optionals, and#expectassertion syntax is consistent with the new testing framework.Tests/AblyChatTests/DefaultRoomsTests.swift (5)
4-4: LGTM!Changing
DefaultRoomsTestsfrom a class to a struct aligns with the conventions of the new testing framework. Structs are value types and are more lightweight compared to reference types like classes.
Line range hint
6-21: LGTM!The changes to the
get_returnsRoomWithGivenIDmethod align with the conventions of the new testing framework:
- Renaming the method and adding the
@Testannotation follows the new syntax.- Updating the assertions to use
#requireand#expectmaintains the test logic while conforming to the new framework.The core functionality of the test remains intact, ensuring that the
getmethod returns a room with the given ID and options.
Line range hint
25-39: LGTM!The changes to the
get_returnsExistingRoomWithGivenIDmethod align with the conventions of the new testing framework:
- Renaming the method and adding the
@Testannotation follows the new syntax.- Updating the assertion to use
#expectmaintains the test logic while conforming to the new framework.The core functionality of the test remains intact, ensuring that the
getmethod returns the same room object when called with the same room ID.
Line range hint
43-65: LGTM!The changes to the
get_throwsErrorWhenOptionsDoNotMatchmethod align with the conventions of the new testing framework:
- Renaming the method and adding the
@Testannotation follows the new syntax.- Updating the assertion to use
#expectmaintains the test logic while conforming to the new framework.The core functionality of the test remains intact, ensuring that the
getmethod throws aninconsistentRoomOptionserror when called with different options for the same room ID.
65-65: LGTM!Using the
isChatErrorfunction in the assertion improves the readability and maintainability of the test. It encapsulates the logic of checking the error type and code, making the assertion more concise.Tests/AblyChatTests/DefaultRoomTests.swift (13)
3-3: Switch to Swift Testing frameworkImporting the
Testingframework aligns with the migration to Swift Testing, as outlined in the PR objectives.
5-5: Updated test case to a structConverting
DefaultRoomTestsfrom a class to a struct is appropriate in Swift Testing and reflects the new testing conventions.
6-6: Adopting@TestannotationUsing the
@Testattribute correctly marks the function as a test case in Swift Testing.
7-7: Properly defined asynchronous test functionThe test function
attach_attachesAllChannels_andSucceedsIfAllSucceedis correctly defined withasync throws, suitable for asynchronous testing.
30-30: Asserting channel attach invocationThe assertion
#expect(channel.attachCallCounter.isNonZero)appropriately verifies that theattachmethod was called on each channel.
33-34: Correct use of asynchronous assertionsThe use of
await #expectandtry await #expectensures proper verification of asynchronous operations and state changes.
37-37: Using@Testannotation for test functionThe
@Testattribute is appropriately applied toattach_attachesAllChannels_andFailsIfOneFails.
65-65: Verify error comparison using===Ensure that using
===for error comparison is intentional. The===operator checks for reference equality (same instance). If you need to compare the error contents, consider comparing specific properties likecodeormessage, or use==ifARTErrorInfoconforms toEquatable.
68-68: Using@TestannotationThe
@Testattribute correctly marks the test functiondetach_detachesAllChannels_andSucceedsIfAllSucceed.
92-92: Asserting channel detach invocationThe assertion
#expect(channel.detachCallCounter.isNonZero)correctly verifies that thedetachmethod was called on each channel.
95-96: Correct use of asynchronous assertionsThe use of
await #expectandtry await #expectensures proper verification of asynchronous operations and state changes.
99-99: Using@TestannotationThe
@Testattribute correctly marks the test functiondetach_detachesAllChannels_andFailsIfOneFails.
127-127: Duplicate: Verify error comparison using===This error comparison is similar to the one commented on earlier. Ensure the use of
===is intentional.
lawrence-forooghian
force-pushed
the
55-switch-to-swift-testing
branch
from
ca271d6
to
9a3af23
Compare
lawrence-forooghian
force-pushed
the
55-switch-to-swift-testing
branch
from
9a3af23
to
c3cf9b8
Compare
|
@maratal just a heads up that I’ve changed something since you reviewed this (see the final part of the commit message, about async stuff) |
I think that "expect -> async" vs "async -> expect" is a minor difference and can be changed later with one find/replace command. |
We’re doing this because it’s, I guess, the future, and offers
performance improvements, but more immediately because it reduces
verbosity when testing `async` functions and properties.
I’ve done the migration in a pretty straightforward fashion, just
stripping the `test` prefix from the method names (which my eyes are
still getting used to), and not trying to use any new features like test
descriptions, nested suites, or tags. We can figure out how we want to
use these as we get used to using Swift Testing.
Decided to keep testing for errors via a do { … } catch { … } instead of
using #expect(…, throws:) because I like being able to write tests in a
Given / When / Then fashion — i.e. do things, then make assertions.
Whilst working on this, I noticed that some of Swift Testing’s useful
test failure messages stop being so useful when testing asynchronous
code, which takes a bit of the shine off it. For example, if you write
> #expect(status.current == .attached)
then you’ll get a failure message of
> Expectation failed: (current → .detached) == .attached
that is, it shows you information about `current` which helps you to
understand why the expectation failed.
But if, on the other hand, you write
> #expect(await status.current == .attached)
then you’ll get a failure message of
> Expectation failed: await status.current == .detached
which is less useful. I asked about this in [1] and was told that it’s a
known issue and that Swift macro limitations mean it’s unlikely to be
fixed soon. ([2] is a similar question that I found after.) If we decide
that this is a dealbreaker and that we want the rich failure messages,
then we’ll need to switch back to the current way of doing things; that
is, first do the `await` and then the #expect.
Resolves #55.
[1] https://forums.swift.org/t/expectation-failure-messages-are-less-useful-with-await/74754
[2] https://forums.swift.org/t/try-expect-throwing-or-expect-try-throwing/73076/17
lawrence-forooghian
force-pushed
the
55-switch-to-swift-testing
branch
from
c3cf9b8
to
f5cab27
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
Tests/AblyChatTests/InternalLoggerTests.swift (1)
12-12: LGTM: Assertions updated to Swift Testing syntaxThe assertions have been correctly updated to use the #expect and #require syntax of the Swift Testing framework. The test cases remain the same, ensuring that the functionality is still properly verified.
Consider adding a small comment explaining the purpose of the #require call, as it might not be immediately clear to all readers why it's used instead of #expect. For example:
// Use #require to unwrap the optional and fail the test if it's nil let receivedArguments = try #require(logger.logArguments)Also applies to: 14-16
Tests/AblyChatTests/DefaultInternalLoggerTests.swift (1)
33-34: Consider breaking down long method names for improved readabilityWhile the current implementation is correct and follows the new framework's conventions, consider breaking down long method names into multiple lines for improved readability. For example:
@Test func log_whenLogLevelArgumentIsLessSevereThanLogLevelProperty_itDoesNotLog() {Could be written as:
@Test func log_whenLogLevelArgumentIsLessSevereThanLogLevelProperty_itDoesNotLog( ) {This is a minor suggestion and is up to your team's coding style preferences.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- Tests/AblyChatTests/DefaultChatClientTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultInternalLoggerTests.swift (3 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (2 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (5 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (4 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/InternalLoggerTests.swift (1 hunks)
- Tests/AblyChatTests/MessageSubscriptionTests.swift (2 hunks)
- Tests/AblyChatTests/SubscriptionTests.swift (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- Tests/AblyChatTests/DefaultChatClientTests.swift
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/SubscriptionTests.swift
Additional context used
Learnings (1)
Tests/AblyChatTests/DefaultRoomTests.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 not posted (24)
Tests/AblyChatTests/InternalLoggerTests.swift (4)
2-2: LGTM: Import statement updated for Swift TestingThe import statement has been correctly updated to use the new testing framework, which aligns with the PR objective of switching to Swift Testing.
5-6: LGTM: Test method correctly annotated and renamedThe test method has been properly annotated with @test, and the 'test' prefix has been removed from the method name. This change aligns with the new testing framework and the PR objectives.
1-18: Overall changes align well with PR objectivesThe changes in this file successfully transition the test from XCTest to Swift Testing. The structure has been updated (class to struct, method annotation), and assertions have been modified to use the new syntax. These changes align perfectly with the PR objectives of switching to Swift Testing and improving the testing of asynchronous functions.
The core functionality of the test remains intact, ensuring that the
MockInternalLoggercorrectly populates the log message's level, message, and code location.
4-4: LGTM: Test class changed to structThe test class has been correctly changed to a struct, which is consistent with the new Swift Testing framework's approach. This change removes the inheritance from XCTestCase, as it's no longer needed.
Let's verify that this change has been consistently applied across all test files:
Tests/AblyChatTests/DefaultInternalLoggerTests.swift (6)
2-2: LGTM: Import statement updated for new testing frameworkThe import of 'Testing' is consistent with the PR objective to switch to Swift Testing.
4-4: LGTM: Test class converted to structThe change from a class extending XCTestCase to a struct is in line with the new testing framework's conventions.
5-10: LGTM: 'defaults' test method updated to new frameworkThe changes to this test method are consistent with the PR objectives:
- 'test_' prefix removed from the method name.
- @test annotation added.
- Assertions updated to use #expect syntax.
These modifications align well with the transition to the new testing framework.
Line range hint
13-30: LGTM: 'log' test method updated to new frameworkThe changes to this test method are well-implemented and align with the PR objectives:
- 'test_' prefix removed from the method name.
- @test annotation added.
- Assertions updated to use #expect and #require syntax.
- 'Given/When/Then' structure maintained, as mentioned in the PR objectives.
These modifications successfully transition the test to the new framework while preserving the test's logic and structure.
Line range hint
33-50: LGTM: Long-named test method updated to new frameworkThe changes to this test method are well-implemented and consistent with the PR objectives:
- 'test_' prefix removed from the method name.
- @test annotation added.
- Assertion updated to use #expect syntax.
- 'Given/When/Then' structure maintained, as mentioned in the PR objectives.
These modifications successfully transition the test to the new framework while preserving the test's logic and structure.
Line range hint
1-51: LGTM: Consistent and comprehensive transition to new testing frameworkThe changes in this file demonstrate a thorough and consistent transition to the new testing framework:
- All test methods have been updated uniformly.
- The file structure has been changed from a class to a struct as required.
- Assertions have been consistently updated to the new syntax (#expect and #require).
- The original test logic and 'Given/When/Then' structure have been preserved.
These changes successfully meet the PR objectives while maintaining the integrity of the tests.
Tests/AblyChatTests/MessageSubscriptionTests.swift (5)
3-3: LGTM: Import statement for new testing framework added.The addition of
import Testingaligns with the PR objective of switching to Swift Testing. This change is necessary for utilizing the new testing framework in this file.
26-27: LGTM: Test method updated to new framework syntax.The changes to
withMockAsyncSequencealign with the PR objectives:
- Using
@Testannotation instead offuncprefix.- Removing the
testprefix from the method name.- Updating the assertion to use
#expect.These modifications successfully transition the test to the new Swift Testing framework.
Note: As mentioned in the PR summary, the new
#expectsyntax may result in less informative failure messages for asynchronous tests. This is a known trade-off that aligns with current community best practices.Also applies to: 30-30
33-34: LGTM: Emit test method successfully updated.The changes to the
emittest method are consistent with the previous updates:
@Testannotation added.testprefix removed from the method name.- Assertion updated to use
#expect.The core logic of the test remains intact, maintaining its focus on emitting and collecting messages.
Also applies to: 42-42
45-46: LGTM: MockGetPreviousMessages test method successfully updated with new syntax.The changes to
mockGetPreviousMessagesare consistent with the PR objectives:
@Testannotation added andtestprefix removed.XCTUnwrapreplaced with#require.- Assertion updated to use
#expect.The use of
#requireis a new pattern introduced in this method, which appears to be the equivalent ofXCTUnwrapin the new testing framework.The comment explaining the type casting approach to avoid a compiler error is helpful for understanding the implementation choice. Consider documenting this workaround in a more visible place (e.g., README or CONTRIBUTING file) if it's a pattern that needs to be repeated across multiple test files.
Also applies to: 52-53
21-21: LGTM: Test class changed to struct.Changing
MessageSubscriptionTestsfrom a class to a struct aligns with the new Swift Testing framework's approach. This modification is consistent with the PR objectives.Please verify that this change doesn't introduce any unintended side effects in test initialization or management. Run the following script to check for any other test files that might need similar updates:
Tests/AblyChatTests/DefaultRoomTests.swift (9)
3-5: LGTM: Successful migration to Swift Testing frameworkThe changes to import the
Testingframework and convert the test class to a struct align with the migration to Swift Testing. This is a good step towards modernizing the test suite.
6-7: LGTM: Test method declaration updated to Swift Testing syntaxThe use of
@Testannotation and removal of the 'test_' prefix from the method name adhere to Swift Testing conventions. This improves readability and aligns with modern Swift testing practices.
30-34: Assertion syntax updated, but consider async error message clarityThe update to use
#expectinstead of XCTAssert* methods is correct for Swift Testing. However, as you've noted in the PR description, usingawaitinside#expectmay lead to less informative error messages for async tests. This is a known limitation of the current Swift Testing implementation.Consider monitoring the Swift forums for updates on this issue or explore alternative patterns if more detailed error messages become crucial for debugging.
37-38: LGTM: Test method declaration updated correctlyThe second test method has been properly updated to use the
@Testannotation and the 'test_' prefix has been removed from the method name. This change is consistent with Swift Testing conventions and improves readability.
65-65: LGTM: Assertions updated correctlyThe assertion has been properly updated to use
#expectand#require, which is the correct syntax for Swift Testing. The use of#requirefor unwrapping the optional value before the assertion is a good practice.
68-69: LGTM: Test method declaration updated correctlyThe third test method has been properly updated to use the
@Testannotation and the 'test_' prefix has been removed from the method name. This change is consistent with Swift Testing conventions and improves readability.
92-96: Assertion syntax updated, but consider async error message clarityThe assertions have been correctly updated to use
#expectinstead of XCTAssert* methods, which is appropriate for Swift Testing. However, as noted earlier, usingawaitinside#expectmay result in less informative error messages for async tests.While this is a known limitation of the current Swift Testing implementation, it's worth keeping an eye on potential improvements or workarounds in future updates.
99-100: LGTM: Test method and assertions updated correctlyThe fourth test method has been properly updated:
- The
@Testannotation is used correctly.- The 'test_' prefix has been removed from the method name, improving readability.
- The assertion has been updated to use
#expectand#require, which is the correct syntax for Swift Testing.These changes are consistent with Swift Testing conventions and represent a successful migration of the test suite.
Also applies to: 127-127
Line range hint
1-128: Overall: Successful migration to Swift Testing frameworkThe changes in this file demonstrate a comprehensive and consistent migration from XCTest to the Swift Testing framework. Key points:
- All test methods have been properly annotated with
@Testand renamed according to Swift Testing conventions.- Assertions have been updated to use
#expectand#requiresyntax.- The test class has been converted to a struct, aligning with Swift Testing practices.
The migration addresses the PR objectives, including the transition to Swift Testing and handling of async testing syntax. The known limitation regarding less informative error messages for async tests has been acknowledged and is being monitored for future improvements.
Great job on implementing these changes consistently throughout the file!
Resolves #55. See commit message for more details.
Currently in the middle of some back-and-forth on whether it's OK to write— done, been told it’s best not to do that so have switched it toawait #expect(…)(see Swift Forums issue linked to in commit message), but I think people can take a look at this PR anyway.#expect(await …), accepting the poorer failure messages.Summary by CodeRabbit
Summary by CodeRabbit
Refactor
@Testannotations.Chores