SyncUps Tutorial: Removing attendee test does not pass

[tomassliz]

Description

In the Section 1 Removing attendees there is a test called testRemoveAttendee that will fail even though you have followed the instructions. The issue is that the focus property value changes unexpectedly:

A state change does not match expectation: …

      SyncUpForm.State(
    −   _focus: .title,
    +   _focus: .attendee(
    +     Tagged(rawValue: UUID(1580A789-51EF-4257-86AF-4D297EB64CF0))
    +   ),
        _syncUp: SyncUp(…)
      )

(Expected: −, Actual: +)

There is also a note
I'm not sure how you want to drive the tutorial so I just open this issue without a fix.

Checklist

• I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
• If possible, I've reproduced the issue using the main branch of this package.
• This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

The test should pass.

Actual behavior

The test doesn't pass.

Steps to reproduce

No response

The Composable Architecture version information

No response

Destination operating system

iOS 17

Xcode version information

15.4

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0

[stephencelis]

@tomassliz That failure looks like maybe line 24 of step 5 is missing:

$0.focus = .attendee(attendee2.id)

That line should make things pass. Since this is a tutorial step and not a bug in the library, I'm going to convert to a discussion, but let us know if that fixes things for you or if there's still a problem!

[tomassliz]

@stephencelis I was not blocked by this and I fixed it this way. But I thought it should be fixed in the tutorial itself.

[stephencelis]

@tomassliz I believe the tutorial is correct, though. As long as that assertion is present the test should pass.

[tomassliz]

@stephencelis I believe step 3 is incorrect because the test does not contain the assertion. Although the test is not intended to test the logic of focus, it is exhaustive.

[jwhitley]

I just hit precisely this test failure, and agree that this appears to be a bug in the tutorial. At the state of the code at Testing the sync-up form > Removing attendees, Step 3, I see the same test failure where the focus field wasn't updated from .title to .attendee(...). @stephencelis, can you clarify why this shouldn't be an error? The state update in the assertion block is only $0.syncUp.attendees.removeFirst(), which won't match the corresponding focus field update in the reducer. And that's exactly what the failure state diff shows.

[gbuela]

I hit this too. From this comment that shows further below in the same tutorial page, I reckon that the intended logic is not implemented in the reducer: What happens when there are two attendees, neither of which is focused, and you delete one? We would expect that the focus does not change, but the best way to prove that is to write a test.

So in testRemoveAttendee() we don't have an explicit focus set, so the SyncUpForm.State defaults to focus on .title. Which means that the attendee we're deleting is not focused and we should not expect the focus to move to the remaining attendee. For this reason I get that the test is OK but the reducer is not doing what it's meant to.

Making this change in the reducer I get the test to pass.

                if let focus = state.focus,
                   case State.Field.attendee = focus {
                   // change attendee focus only if an attendee was focused
                    let index = min(firstIndex, state.syncUp.attendees.count - 1)
                    state.focus = .attendee(state.syncUp.attendees[index].id)
                }