-
-
Notifications
You must be signed in to change notification settings - Fork 54
Refactor all Tests from XCTest to Swift Testing #208
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
base: main
Are you sure you want to change the base?
Conversation
…ed Tags to all tests and Suites for clarity. Cleaned up old comments. Added all folders called `.docc-build` to the `.gitignore` if documentation is generated locally
* Correction to SwiftCrossUIBackendTests.swift * Corrections for SwiftCrossUIInternalsTests.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping to transition SwiftCrossUI to Swift Testing! I've some comments requesting various changes.
In addition to the self contained changes I've requested, I would also like the tests to be rearranged into 3 suites, rather than the 2 suites that you've rearranged them into (which aren't a correct categorisation). The suite names I'd use are; BackendTests
, StateTests
and NavigationPathTests
. The basic layout test should go in BackendTests
, the two state tests should go in StateTests
and the remaining navigation path codable test should go in NavigationPathTests
.
Please limit your use of AI in future PRs, and when addressing these comments, as it makes some strange decisions that I don't believe you would've made by yourself (e.g. the code around line 68 of SwiftCrossUIInternalsTests.swift). Submissions made with AI generally take me significantly longer to review, and have an increased chance that I'll have to request changes.
case FailedBitmapBack | ||
case FailedTiffRep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use lower camel case for enum cases (e.g. failedBitmapBack
instead of FailedBitmapBack
).
That said, due to test errors being purely for reporting, I think this error type would be better suited as a struct with a message
field. Then tests can easily report detailed errors without having to create enum cases and update the switch (which adds a bit of a barrier to changing tests).
throw BackendTestError(message: "Failed to create backing bitmap")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I found an option where you can treat it like a enum error while still using the struct type. It also has a .new("")
function for explicitly declaring them in functions with a explicit throw type. If you don't like this idea just let me know
I forgot to add, the tests currently don't compile (due to around 14 compilation errors). |
The Line you referenced was just a byproduct of 3 hours of trying to make that function be completely parameterized. I will just remove it. I will be working on improving clarity before sending in a commit. I can understand that that was really a confusing section. |
This is mostly because of the conditional compilation on my end disabling the tests inside of the Backend Suite. Removing as much of the conditionally compiled tests should be a future goal to improve these issues |
/// Specifies the values that are saved into the `NavigationPath` | ||
let Values: [any Codable] = [ | ||
"a", | ||
1, | ||
[1, 2, 3], | ||
5.0, | ||
] | ||
|
||
// All types are required to be a type of `Codable` to permit encoding it and `Equatable` to satisfy ``compareComponents(ofType:,_:,_:)`` | ||
/// Specifies the types used for indicating the `destinationTypes` paramater of `NavigationPath.path(destinationTypes:)` | ||
/// Be aware that you still need to add a new expect block for each unique value | ||
let Types: [any Codable.Type] = | ||
[ | ||
String.self, Int.self, [Int].self, Double.self, | ||
] as! [any Codable.Type] | ||
|
||
try #require( | ||
Values.count == Types.count, | ||
""" | ||
Test `CodableNavigationPath` is Malformed. | ||
`Values` and `Types` must have a 1 to 1 match-up | ||
""" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a potentially more succinct variant of the test setup
/// Specifies the values that are saved into the `NavigationPath` | |
let Values: [any Codable] = [ | |
"a", | |
1, | |
[1, 2, 3], | |
5.0, | |
] | |
// All types are required to be a type of `Codable` to permit encoding it and `Equatable` to satisfy ``compareComponents(ofType:,_:,_:)`` | |
/// Specifies the types used for indicating the `destinationTypes` paramater of `NavigationPath.path(destinationTypes:)` | |
/// Be aware that you still need to add a new expect block for each unique value | |
let Types: [any Codable.Type] = | |
[ | |
String.self, Int.self, [Int].self, Double.self, | |
] as! [any Codable.Type] | |
try #require( | |
Values.count == Types.count, | |
""" | |
Test `CodableNavigationPath` is Malformed. | |
`Values` and `Types` must have a 1 to 1 match-up | |
""" | |
) | |
/// Specifies the values and types input into the `NavigationPath` | |
let typeValuePairs: [(type: any Codable.Type, value: any Codable)] = | |
[ | |
(String.self, "a"), | |
(Int.self, 1), | |
([Int].self, [1, 2, 3]), | |
(Double.self, 5.0), | |
] as! [(any Codable.Type, any Codable)] | |
let Values: [any Codable] = typeValuePairs.map { $0.value } | |
let Types: [any Codable.Type] = typeValuePairs.map { $0.type } |
#expect( | ||
Self.compareComponents(ofType: String.self, components[0], decodedComponents[0]), | ||
"An Issue with Navigation path data retainment occured" | ||
) | ||
#expect( | ||
Self.compareComponents(ofType: Int.self, components[1], decodedComponents[1]), | ||
"An Issue with Navigation path data retainment occured" | ||
) | ||
#expect( | ||
Self.compareComponents(ofType: [Int].self, components[2], decodedComponents[2]), | ||
"An Issue with Navigation path data retainment occured" | ||
) | ||
#expect( | ||
Self.compareComponents(ofType: Double.self, components[3], decodedComponents[3]), | ||
"An Issue with Navigation path data retainment occured" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider using a loop but it would have been excessive and overkill as there is only one instance of each type
Changes:
@Test
.gitignore
DocC build artifacts and the.fleet
folderAdditional Notes
DefaultBackend
work as a mock backend for the purpose of removing theAppKitBackend
requirement for the contents of theBackend
Suite?Fixes #196