Add a testcase for HTTPCookieStorage.description #1359
Conversation
| @@ -32,6 +32,7 @@ class TestHTTPCookieStorage: XCTestCase { | |||
| ("test_cookiesForURL", test_cookiesForURL), | |||
| ("test_cookiesForURLWithMainDocumentURL", test_cookiesForURLWithMainDocumentURL), | |||
| ("test_cookieInXDGSpecPath", test_cookieInXDGSpecPath), | |||
| ("test_descriptionCookie", test_descriptionCookie) | |||
There was a problem hiding this comment.
This needs a comma at the end, so it doesn't cause unnecessary noise later
|
Please remove the |
| @@ -246,6 +252,23 @@ class TestHTTPCookieStorage: XCTestCase { | |||
| XCTAssertEqual(storage.cookies(for: url1!)!.count, 0) | |||
| } | |||
|
|
|||
| func descriptionCookie(with storageType: _StorageType) { | |||
There was a problem hiding this comment.
As this function is only used inside test_descriptionCookie() why not make it a nested function?
There was a problem hiding this comment.
@ianpartridge I see that this pattern (calling a function XXX two times from test_XXX) is used in all the tests in TestHTTPCookieStorage. Do you think doing this as a separate PR will make more sense?
| @@ -246,6 +252,23 @@ class TestHTTPCookieStorage: XCTestCase { | |||
| XCTAssertEqual(storage.cookies(for: url1!)!.count, 0) | |||
| } | |||
|
|
|||
| func descriptionCookie(with storageType: _StorageType) { | |||
| let storage = getStorage(for: storageType) | |||
| XCTAssertEqual(storage.description, "<NSHTTPCookieStorage cookies count:\(storage.cookies!.count)>") | |||
There was a problem hiding this comment.
I know this is done elsewhere in the tests but I don't like force unwrapping. If it fails, the whole testsuite blows up. It's better to do:
guard let cookies = storage.cookies else {
XCTFail("No cookies")
return
}
etc...
vipinmenon
force-pushed
the
cookieStorageTest
branch
from
53fcee7 to
ab3b13c
Compare
|
@ianpartridge @alblue Thanks for the review. I have responded to the comments. Kindly approve. |
|
@swift-ci please test |
pushkarnk
changed the title
|
I see that all of the comments have been addressed. @swift-ci please test and merge |
Per the guidance in #1313 this PR splits out the test for
HTTPCookieStorage.descriptioncc @ianpartridge