Skip to content

Conversation

brunoocasali
Copy link
Member

  • Remove all keys before starting a test to prevent order-dependency in the tests
  • Make description an optional value

@brunoocasali brunoocasali added the enhancement New feature or request label Aug 2, 2022
@brunoocasali brunoocasali requested a review from bidoubiwa August 2, 2022 20:32
*/
public struct KeyParams: Codable, Equatable {
public let description: String
public var description: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, why are some var and some let? We cant change actions ? Is this because of updateKey ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct!

self.client.deleteKey(key: $0.uid) { result in
switch result {
case .success:
()
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the same as keyExpectation.fulfill() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we define a switch, we should provide an exhaustive list of possibilities, such as success and failure. The problem is that Swift requires me to provide at least a statement when defining a case. Since this method is meant to be a setup and I'm not interested in the success, I could provide the () empty function, but I would like to know if there is another way to handle that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if you are not interested in the success is .fulfill() not used to stop the wait of the expectation?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but that's the point, this code is written inside of the setup(), so it's not meant to be a "test" so in other words, no expectation should be needed 😬

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

Base automatically changed from task-info-uid to bump-meilisearch-v0.28.0 August 4, 2022 13:28
@brunoocasali brunoocasali merged commit 3605396 into bump-meilisearch-v0.28.0 Aug 4, 2022
@brunoocasali brunoocasali deleted the improve-key-tests branch August 4, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants