Skip to content

Conversation

@kvvzr
Copy link
Contributor

@kvvzr kvvzr commented Mar 16, 2024

Added first tests using TestStore.

Ref: #28

Copy link
Member

@d-date d-date left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! And I ask you to fix some points:

  • Why you back to KeyPath style: @DependencyClient will generate testValue().
  • Dummy data should be under your tests if you use only for testing.

@kvvzr
Copy link
Contributor Author

kvvzr commented Mar 17, 2024

I have fixed the pointed out issues.

One point of concern is that I named it 'mock' based on the sample code from TCA, but thinking about it, 'dummy' might be a better choice. What do you think?

Comment on lines 45 to 46
static public let testValue = Self()

Copy link
Member

Choose a reason for hiding this comment

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

This is also macro's roll

case path(StackAction<Path.State, Path.Action>)
case destination(PresentationAction<Destination.Action>)
case view(View)
case fetchResponse(Result<(Conference, Conference, Conference), Error>)
Copy link
Member

Choose a reason for hiding this comment

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

Might be better with creating struct.

Suggested change
case fetchResponse(Result<(Conference, Conference, Conference), Error>)
case fetchResponse(Result<SchedulesResponse, Error>)

The struct may be like below:

public struct SchedulesResponse: Equatable {
  var day1: Conference
  var day2: Conference
  var workshop: Conference
}

@d-date
Copy link
Member

d-date commented Mar 17, 2024

One point of concern is that I named it 'mock' based on the sample code from TCA, but thinking about it, 'dummy' might be a better choice. What do you think?

Either is OK in this test.

Copy link
Member

@d-date d-date left a comment

Choose a reason for hiding this comment

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

Thank you for adding test! Now we're time to run it on CI!

@d-date d-date merged commit eef1a4d into tryswift:main Mar 18, 2024
@kvvzr
Copy link
Contributor Author

kvvzr commented Mar 18, 2024

Thank you for your review!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants