Skip to content

Conversation

acosmicflamingo
Copy link
Contributor

Addresses #309.

The issue is that the dismiss closure being passed the controller to be dismissed, but the actual area where the dismissal occurs is using self instead of the respective controller. At least, that's the theory...let's see if tests pass. I was able to at least fix the issue that was in #309 with it.

@acosmicflamingo acosmicflamingo changed the title Dismiss should be called on child being passed in onDismiss closure Dismiss should be called on child being passed in dismiss closure Oct 1, 2025
@acosmicflamingo
Copy link
Contributor Author

Aww, two failures :(

/CaseStudiesTests/PresentationTests.swift:426: error: -[CaseStudiesTests.PresentationTests testDismissMiddlePresentation] : XCTAssertNil failed: "<_TtCFC16CaseStudiesTests17PresentationTests29testDismissMiddlePresentationFzZT_T_L_2VC: 0x103cec910>"
/CaseStudiesTests/PresentationTests.swift:372: error: -[CaseStudiesTests.PresentationTests testDismissMultiplePresentations] : XCTAssertNil failed: 
"

@acosmicflamingo
Copy link
Contributor Author

The failure was in the flaky PresentationTests.testPushViewController_ManualPop() test (#304)!!!

Going to clean this PR up but it looks like the source of the issues was treating dismissing self as if it was dismissing what was presented.

@acosmicflamingo
Copy link
Contributor Author

GAH; it re-introduced the problem in #309

@acosmicflamingo
Copy link
Contributor Author

Yay! The flaky test is what failed, and it does fix #309.

@mbrandonw @stephencelis would you be willing to entertain the idea that the test suite contain UI testing? The problem I face with creating a test that catches #309 is that everything works as expected when controlling navigation programatically. There is a difference in execution of steps between programmatically dismissing a child view controller, and swiping down (or tapping the "Ok" button on an alert), and I think only UI testing can capture these corner cases.

@StuSharpe
Copy link

Hi @acosmicflamingo, as discussed here: #309 (comment)

4f3baae doesn't fix the crash issue, which I believe is a seperate bug to what this PR is addressing. Happy for this PR to be merged.

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