Skip to content

[SR-1702] NSJSONSerialization.data(...) fatalErrors on bad input #1151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

glenna
Copy link
Contributor

@glenna glenna commented Aug 1, 2017

SR-1702 NSJSONSerialization.data(withJSONObject:options:) should fatalError on bad input not throw

-removed the throw in the case of using a stream: in both cases in objc it is an NSInvalidArgumentException

-removed the test checking for the throw in the case of the stream (and, AFAIK, there's no good way to test for fatalErrors, but happy to be corrected on that fact)

@hartbit
Copy link
Contributor

hartbit commented Aug 1, 2017

Its a pity that we loose the unit-test :-/ Is there anything we can do about it?

@glenna glenna force-pushed the fix/fatalerror_on_jsonserialization_bad_input branch from 3750b32 to 57062ee Compare August 3, 2017 18:48
@glenna
Copy link
Contributor Author

glenna commented Aug 4, 2017

@phausler can you take a look? also any guidance about the tests?

@phausler
Copy link
Contributor

phausler commented Aug 4, 2017

Sadly we do not yet have a way to validate fatal errors in XCTest. Shy of perhaps launching a NSTask to spawn the test in a sub-process.

Overall this looks like a matching behavior to the objc counterpart.

@phausler
Copy link
Contributor

phausler commented Aug 4, 2017

@swift-ci please test

1 similar comment
@ianpartridge
Copy link
Contributor

@swift-ci please test

@glenna
Copy link
Contributor Author

glenna commented Aug 10, 2017

seems just the build timed out @ianpartridge

@phausler
Copy link
Contributor

@swift-ci please test and merge

2 similar comments
@alblue
Copy link
Contributor

alblue commented Aug 11, 2017

@swift-ci please test and merge

@ianpartridge
Copy link
Contributor

@swift-ci please test and merge

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.

6 participants