Fix crash when using UIWindow in assertSnapshot
#366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`prepareView` in `View.swift` will now check if the current snapshot target is a `UIWindow`. If that condition is `true`, then it will return early and use the window as its dispose value. It will make the assumption that the window has a `rootViewController` set and use that as it's `viewController`. This avoids unbalanced calls to appearance transitions during test teardown which will throw exceptions and crash the test.
the second time around.
Rely on configuration options instead.
zenangst
force-pushed
the
fix/crash-when-using-uiwindow-in-assertSnapshot
branch
from
39c79e2
to
e04b8e1
Compare
|
@stephencelis mind taking a look a this? 😎 |
|
Hey @zenangst! Thanks for taking the time to submit this and sorry if things are taking a bit longer to take a look at. Given the early out does this mean that the traits and sizing will not be applied to the window? |
|
Yeah traits and sizing should be left out. Maybe that is a false assumption on my part but if you give it the window then everything down the line should already be handled in your test. What do you think? |
|
@stephencelis I pushed another commit to include the traits if the root view controller of the window could be resolved. |
zenangst
commented
Aug 13, 2020
| if let rootViewController = window.rootViewController { | ||
| return add(traits: traits, viewController: rootViewController, to: window) | ||
| } else { | ||
| return {} |
There was a problem hiding this comment.
We could add an assertFailure here
|
ping @stephencelis 😎 |
|
Any headway here? Something that needs changing before it can be merged? 🌞 |
schalkwijk
pushed a commit
to schalkwijk/swift-snapshot-testing
that referenced
this pull request
Sep 6, 2020
This way, we don't accidentally hit bad transition points in tests, as per pointfreeco#366. Basically, because we're necessarily running our tests inside of a hosted application, the snapshot library incorrectly creates yet another window in which to run tests, which confuses the ... iOS runtime? or something and causes the app to queue up a bunch of appear / disappear transitions that clog up the main thread until they just halt the entire app, causing the app to stop for a solid 20-30 seconds, which explains timeout issues we're having in CI and occasionally locally. The cost of this is that the `onAppear` triggers no longer trigger in snapshot tests, but that's a small price to pay to make sure the tests actually pass in CI.
schalkwijk
pushed a commit
to schalkwijk/swift-snapshot-testing
that referenced
this pull request
Sep 7, 2020
This way, we don't accidentally hit bad transition points in tests, as per pointfreeco#366. Basically, because we're necessarily running our tests inside of a hosted application, the snapshot library incorrectly creates yet another window in which to run tests, which confuses the ... iOS runtime? or something and causes the app to queue up a bunch of appear / disappear transitions that clog up the main thread until they just halt the entire app, causing the app to stop for a solid 20-30 seconds, which explains timeout issues we're having in CI and occasionally locally. The cost of this is that the `onAppear` triggers no longer trigger in snapshot tests, but that's a small price to pay to make sure the tests actually pass in CI.
schalkwijk
pushed a commit
to schalkwijk/swift-snapshot-testing
that referenced
this pull request
Sep 7, 2020
This way, we don't accidentally hit bad transition points in tests, as per pointfreeco#366. Basically, because we're necessarily running our tests inside of a hosted application, the snapshot library incorrectly creates yet another window in which to run tests, which confuses the ... iOS runtime? or something and causes the app to queue up a bunch of appear / disappear transitions that clog up the main thread until they just halt the entire app, causing the app to stop for a solid 20-30 seconds, which explains timeout issues we're having in CI and occasionally locally. The cost of this is that the `onAppear` triggers no longer trigger in snapshot tests, but that's a small price to pay to make sure the tests actually pass in CI.
schalkwijk
pushed a commit
to schalkwijk/swift-snapshot-testing
that referenced
this pull request
Sep 7, 2020
This way, we don't accidentally hit bad transition points in tests, as per pointfreeco#366. Basically, because we're necessarily running our tests inside of a hosted application, the snapshot library incorrectly creates yet another window in which to run tests, which confuses the ... iOS runtime? or something and causes the app to queue up a bunch of appear / disappear transitions that clog up the main thread until they just halt the entire app, causing the app to stop for a solid 20-30 seconds, which explains timeout issues we're having in CI and occasionally locally. The cost of this is that the `onAppear` triggers no longer trigger in snapshot tests, but that's a small price to pay to make sure the tests actually pass in CI.
|
Please merge, I also encounter this issue |
|
I merged this with |
okankocyigit
approved these changes
Nov 11, 2021
|
@stephencelis will you merge this one, we are also having the same issue and it has been more then one year since @zenangst requested this PR? That would be great if you merge, we don't want to continue with forked one, thanks :) |
|
We have those changes on our fork for a while now and it works perfectly. Maybe this could be somehow merged? 🐱🙏 |
tahirmt
added a commit
to tahirmt/swift-snapshot-testing
that referenced
this pull request
Sep 22, 2022
tahirmt
added a commit
to tahirmt/swift-snapshot-testing
that referenced
this pull request
Sep 22, 2022
thedavidharris
pushed a commit
to thedavidharris/swift-snapshot-testing
that referenced
this pull request
Mar 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
prepareViewinView.swiftwill now check if the current snapshottarget is a
UIWindow. If that condition istrue, then it will returnearly and use the window as its dispose value. It will make the assumption
that the window has a
rootViewControllerset and use that as it'sviewController.This avoids unbalanced calls to appearance transitions during test teardown
which will throw exceptions and crash the test.