Improved Code Coverage #10

Adamcbrz · 2018-01-31T15:17:35Z

I worked on getting code coverage above 90%. Let me know any suggestions or fixes.

achappell · 2018-01-31T15:20:57Z

Tests/BackendServiceTests.swift

@@ -82,6 +82,11 @@ class BackendServiceTests: XCTestCase {
        XCTAssertEqual(mockNetworkService.cancelAllTasksCallCount, 1)
    }

+    func test_BackendServiceError_Equatable() {


is this method necessary?

No, removing

achappell · 2018-01-31T15:23:33Z

Tests/NetworkRequestTests.swift

@@ -104,4 +104,73 @@ class NetworkRequestTests: XCTestCase {
        XCTAssertEqual(urlRequest.cachePolicy, cachePolicy, file: file, line: line)
        XCTAssertEqual(urlRequest.timeoutInterval, timeout, file: file, line: line)
    }
+
+//    func test_NetworkRequestQueryParameterEncodingStrategy_EncodeUrlQueryAllowedCharacterSet() {


Probably want to remove commented out code?

achappell · 2018-01-31T15:24:37Z

Tests/NetworkServiceTests.swift

+
+        let _ = NetworkService(session: URLSession.shared, networkActivityIndicatable: MockNetworkActivityIndicator())
+
+        XCTAssert(true)


this should probably test that the object created is not nil

achappell · 2018-01-31T15:24:48Z

Tests/NetworkServiceTests.swift

+    func test_NetworkServiceHelper_InvalidHTTPResponsErrorUnknownError() {
+        let _ = NetworkServiceHelper.networkServiceFailure(for: NSError(domain: NSURLErrorDomain, code: NSURLErrorBadURL, userInfo: nil))
+
+        XCTAssert(true)


same here, should probably test that the object is not nil

adding a check for the error

achappell · 2018-01-31T15:25:11Z

Tests/BackendServiceTests.swift

+
+        let _ = AnyError(BackendServiceError.networkError(.unknownError, nil))
+
+        XCTAssert(true)


should test that the object created is not nil

The method doesn't return an optional so it either works or it doesn't. I can add a check for nil but it will throw a warning that it will never be nil.

sorry, I'm not super familiar with how this works, is there a property or something that you can test that got set properly, due to initialization?

adding a check for the errror

wmcginty

I had a few style nit picks, but those can be cleaned up when SwiftLint is re-enabled. Looks good to me.

wmcginty · 2018-01-31T19:33:31Z

Sources/Hyperspace/Service/Backend/BackendServiceProtocol.swift

@@ -87,6 +87,6 @@ extension BackendServiceError: Equatable {

 extension AnyError: BackendServiceErrorInitializable {
    public init(_ backendServiceError: BackendServiceError) {
-        self.init(backendServiceError)
+        self.init( backendServiceError as Error)


Nit picking here - but can we remove the leading space?

wmcginty · 2018-01-31T19:34:20Z

Tests/BackendServiceTests.swift

+    func test_BackendServiceProtocol_Execute() {
+        let mock = MockBackendService()
+
+


Extra line of spacing here as well

wmcginty · 2018-01-31T19:34:28Z

Tests/BackendServiceTests.swift

+
+            XCTAssert(true)
+        }
+


tylermilner

Overall, very good. Thank you for bringing our code coverage > 90%! Just some minor changes, mainly around naming to keep things consistent with the rest of the tests.

tylermilner · 2018-02-01T03:43:26Z

Tests/HTTPTest.swift

+            HTTP.HeaderKey.userAgent: "User-Agent"
+        ]
+
+        items.forEach { (arg) in


Minor stylistic concern. Swift allows for tuples as the parameters passed into a closure so there shouldn't be a need for the extra let below.

items.forEach { (key, value) in

tylermilner · 2018-02-01T03:44:26Z

Tests/HTTPTest.swift

+        ]
+
+
+        items.forEach { (arg) in


Same as above.

tylermilner · 2018-02-01T03:49:25Z

Tests/HTTPTest.swift

+        XCTAssert(dataString == content)
+    }
+
+    func test_HTTPStatus_InitWithCode200() {


You may want to consider changing up the naming of some of these tests. This one pretty much makes sense, but might be able to be improved with something like:

func test_HTTPResponseInitWithCode200_ProducesStatusSuccessOK() {

tylermilner · 2018-02-01T03:58:21Z

Tests/HTTPTest.swift

+        let response = HTTP.Response(code: 200, data: nil)
+        switch response.status {
+        case .success(let status):
+            XCTAssert(status.rawValue == 200)


It may be more beneficial to be asserting on the enum case itself, to make sure the semantic meaning to the programmer is correct rather than just the underlying value. This will allow for some cleanup as well.

let response = HTTP.Response(code: 200, data: nil) XCTAssertTrue(response.status == .success(.ok))

tylermilner · 2018-02-01T04:00:01Z

Tests/HTTPTest.swift

+        }
+    }
+
+    func test_HTTPStatus_InitWithCode299() {


Naming tweaks could apply to the rest of these as well.

func test_HTTPResponseInitWithCode299_ProducesStatusSuccessUnknown() {

tylermilner · 2018-02-01T04:12:29Z

Tests/NetworkRequestTests.swift

+        var body: Data?
+    }
+
+    func test_NetworkRequest_GetCachePolicy() {


I might tweak the name of these slightly as well since you're really testing that a NetworkRequest created without an explicit cache policy and timeout return the default cache policy and timeout, right?

func test_NetworkRequestWithoutExplicitCachePolicyAndTimeout_ReturnsDefaultCachePolicyAndTimeout()

tylermilner · 2018-02-01T04:15:12Z

Tests/NetworkRequestTests.swift

+        XCTAssert(true)
+    }
+
+    struct CachePolicyAndTimeOutRequest: NetworkRequest {


Please reorganize these structs and tests so that we have the structs at the top and the rest of the tests above the private methods above.

tylermilner · 2018-02-01T04:23:18Z

Tests/Helper/Mocks/MockNetworkService2.swift

+@testable import Hyperspace
+
+class MockNetworkService2: NetworkService {
+    var setup = false


To make things more clear at the call site, you may want to consider modifying the name of this variable:

private(set) var initWithSessionNetworkActivityControllerWasCalled = false

Another option would be to take a somewhat less explicit name (that still gets the point across):

private(set) var defaultInitWasCalled = false

tylermilner · 2018-02-01T04:24:41Z

Tests/Helper/Mocks/MockNetworkService2.swift

+import Foundation
+@testable import Hyperspace
+
+class MockNetworkService2: NetworkService {


Kind of a personal preference, but to me numbering the class doesn't feel right. What about a name like NetworkServiceMockSubclass (or some variation of that)?

tylermilner · 2018-02-01T04:29:06Z

Tests/NetworkSessionTest.swift

+
+        let networkSession: NetworkSession = URLSession.shared
+        let networkSessionDataTask: NetworkSessionDataTask = networkSession.dataTask(with: defaultRequest, completionHandler: { (data, response, error) in
+        })


Not a big deal, but I think it's a little more concise to just put the closure/function suffix on the same line since there's not actually any implementation. I think you can ignore the parameters passed into the closure as well (with an _).

let networkSessionDataTask: NetworkSessionDataTask = networkSession.dataTask(with: defaultRequest, completionHandler: { _ in })

Adam Brzozowski added 2 commits January 31, 2018 09:05

Fix issue with BackendServiceErrorInitializing calling back into itself

c770f48

Working on improving testing case coverage

e2e9763

achappell reviewed Jan 31, 2018

View reviewed changes

Made updates based on feedback

2abb54c

achappell approved these changes Jan 31, 2018

View reviewed changes

MockNetworkService2 was assigned to the wrong target

3cf750f

wmcginty approved these changes Jan 31, 2018

View reviewed changes

Cleanup whitespace

b59f575

tylermilner suggested changes Feb 1, 2018

View reviewed changes

Renamed some of the tests for better names.

4516060

tylermilner approved these changes Feb 2, 2018

View reviewed changes

tylermilner merged commit a6a6ee8 into BottleRocketStudios:master Feb 2, 2018


		let _ = NetworkService(session: URLSession.shared, networkActivityIndicatable: MockNetworkActivityIndicator())

		XCTAssert(true)


		let _ = AnyError(BackendServiceError.networkError(.unknownError, nil))

		XCTAssert(true)

		func test_BackendServiceProtocol_Execute() {
		let mock = MockBackendService()

Improved Code Coverage #10

Improved Code Coverage #10

Conversation

Adamcbrz commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmcginty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylermilner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamcbrz Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adamcbrz Feb 1, 2018 •

edited

Loading