Testable NetworkRequest #404

timkimadobe · 2023-09-20T23:42:06Z

Description

This PR:

Creates the new TestableNetworkRequest that uses the @testable import AEPServices to gain open inheritable access to the NetworkRequest class.
- TestableNetworkRequest overrides the Equatable and Hashable conformance of NetworkRequest for direct use as a dictionary key using custom logic.
- Equatable and Hashable must have exactly the same results logically for what they consider the "same" element
Uses a new defaultMockResponse in MockNetworkService that creates a default response provided a valid URL
Updates NetworkServiceHelper data structs and methods to account for the new key equality behavior, and refactors usages accordingly
Fixes race condition in MockNetworkService connectAsync implementation by moving the countdown to after the callback is notified, to properly gate test case logic using await

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

… NetworkRequest Override Equatable and Hashable implementations for use in testing comparisons

…orkRequest as keys Refactor usages to use TestableNetworkRequest methods Rename setResponseFor to addResponseFor to more accurately reflect what the method does

…ceHelper

…r alignment

By doing the countdown AFTER notifying the completion handler, awaits on the expected network request can properly gate the rest of the test case logic (for example, if the test case resets the mock network service, there isn't a race condition between the reset and the get mock response, due to prematurely ungated await)

codecov · 2023-09-20T23:50:28Z

Codecov Report

Merging #404 (67bebb0) into dev (c3ce8a6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #404   +/-   ##
=======================================
  Coverage   96.77%   96.77%           
=======================================
  Files          27       27           
  Lines        1671     1671           
=======================================
  Hits         1617     1617           
  Misses         54       54

kevinlind · 2023-09-20T23:51:04Z

Tests/TestUtils/TestableNetworkRequest.swift

+    // will need to be updated accordingly to handle that case.
+
+    // MARK: - Equatable (ObjC) conformance
+    override func isEqual(_ object: Any?) -> Bool {


Add comment stating isEqual only checks URL scheme, host, path and http method, but not the query parameters for equality.

Added comment!

kevinlind · 2023-09-20T23:51:44Z

Tests/TestUtils/TestableNetworkRequest.swift

+    }
+
+    // MARK: - Hashable (ObjC) conformance
+    public override var hash: Int {


Add comment stating hash only includes URL scheme, host, path and http method.

Added comment!

kevinlind · 2023-09-21T00:11:32Z

Tests/TestUtils/NetworkRequestHelper.swift

+    func addResponseFor(networkRequest: NetworkRequest, responseConnection: HttpConnection) {
+        let testableNetworkRequest = TestableNetworkRequest(from: networkRequest)
+        if networkResponses[testableNetworkRequest] != nil {
+            networkResponses[testableNetworkRequest]?.append(responseConnection)
+        }
+        else {
+            networkResponses[testableNetworkRequest] = [responseConnection]
+        }


I think this should still be setResponseFor and the networkResponses type should be [NetworkRequest: HttpConnection] unless you see a need for an array of HttpConnections for a single request. If you see here in MockNetworkRequest, only the first HttpConnection is used which I believe is a side effect of not being able to group similar NetworkRequest objects together. Now with this TestableNetworkRequest, we can just store a single HttpConnection for the set of "equal" NetworkRequests.

I think this is a great point! Looking at the default NetworkService connectAsync implementation, I think there can only ever be a single HttpConnection response for a given request, based on the dataTask.

The only case I can think of multiple connections for the same request is maybe a recoverable failure response with retry? But even then I think we can use staggered/interleaved expectations instead of capturing both with the same key

I've made the following changes:

Reverted the method name change from addResponseFor to setResponseFor

Updated param responseConnection to be HttpConnection?

This has the subtle effect of removing any existing dictionary entry for the given NetworkRequest if the HttpConnection value passed is nil

Updated get__ResponseFor methods to be singular instead of plural

Updated integration test cases to check for single response (non-nil instead of count), and removed array access logic

kevinlind · 2023-09-21T01:52:55Z

Tests/TestUtils/MockNetworkService.swift

@@ -58,7 +56,7 @@ class MockNetworkService: Networking {
    /// Sets the mock `HttpConnection` response connection for a given `NetworkRequest`. Should only be used
    /// when in mock mode.
    func setMockResponseFor(networkRequest: NetworkRequest, responseConnection: HttpConnection?) {
-        helper.setResponseFor(networkRequest: networkRequest, responseConnection: responseConnection)
+        helper.addResponseFor(networkRequest: networkRequest, responseConnection: responseConnection ?? defaultMockResponse(networkRequest.url))


Adding a default isn't strictly needed as because of the else clause in the if let response ... statement above on line 37.

Removed the defaults here, and updated the MockNetworkService connectAsync to own the default directly

kevinlind · 2023-09-21T01:55:01Z

Tests/TestUtils/NetworkRequestHelper.swift

-            .filter { networkRequest.isCustomEqual($0.key) }
-            .map { $0.value }
+    /// Gets all network responses for the given `NetworkRequest`
+    func getResponsesFor(networkRequest: NetworkRequest) -> [HttpConnection]? {


After changing networkResponses to [NetworkRequest: HttpConnection], this can return just HttpConnection? instead of an array.

Updated return type to HttpConnection?

Refactor related logic for getting and setting, and usage sites

…ace it is used

timkimadobe

Thanks so much for the review @kevinlind! Updated based on feedback

timkimadobe · 2023-09-21T20:55:04Z

Tests/TestUtils/NetworkRequestHelper.swift

+    func addResponseFor(networkRequest: NetworkRequest, responseConnection: HttpConnection) {
+        let testableNetworkRequest = TestableNetworkRequest(from: networkRequest)
+        if networkResponses[testableNetworkRequest] != nil {
+            networkResponses[testableNetworkRequest]?.append(responseConnection)
+        }
+        else {
+            networkResponses[testableNetworkRequest] = [responseConnection]
+        }


I think this is a great point! Looking at the default NetworkService connectAsync implementation, I think there can only ever be a single HttpConnection response for a given request, based on the dataTask.

The only case I can think of multiple connections for the same request is maybe a recoverable failure response with retry? But even then I think we can use staggered/interleaved expectations instead of capturing both with the same key

timkimadobe · 2023-09-21T20:57:30Z

Tests/TestUtils/NetworkRequestHelper.swift

-            .filter { networkRequest.isCustomEqual($0.key) }
-            .map { $0.value }
+    /// Gets all network responses for the given `NetworkRequest`
+    func getResponsesFor(networkRequest: NetworkRequest) -> [HttpConnection]? {


Updated return type to HttpConnection?

timkimadobe · 2023-09-21T21:19:04Z

Tests/TestUtils/TestableNetworkRequest.swift

+    // will need to be updated accordingly to handle that case.
+
+    // MARK: - Equatable (ObjC) conformance
+    override func isEqual(_ object: Any?) -> Bool {


Added comment!

timkimadobe · 2023-09-21T21:19:10Z

Tests/TestUtils/TestableNetworkRequest.swift

+    }
+
+    // MARK: - Hashable (ObjC) conformance
+    public override var hash: Int {


Added comment!

timkimadobe · 2023-09-21T21:21:08Z

Tests/TestUtils/NetworkRequestHelper.swift

+    func addResponseFor(networkRequest: NetworkRequest, responseConnection: HttpConnection) {
+        let testableNetworkRequest = TestableNetworkRequest(from: networkRequest)
+        if networkResponses[testableNetworkRequest] != nil {
+            networkResponses[testableNetworkRequest]?.append(responseConnection)
+        }
+        else {
+            networkResponses[testableNetworkRequest] = [responseConnection]
+        }


I've made the following changes:

Reverted the method name change from addResponseFor to setResponseFor

Updated param responseConnection to be HttpConnection?

This has the subtle effect of removing any existing dictionary entry for the given NetworkRequest if the HttpConnection value passed is nil

Updated get__ResponseFor methods to be singular instead of plural

Updated integration test cases to check for single response (non-nil instead of count), and removed array access logic

timkimadobe · 2023-09-21T21:23:34Z

Tests/TestUtils/MockNetworkService.swift

@@ -58,7 +56,7 @@ class MockNetworkService: Networking {
    /// Sets the mock `HttpConnection` response connection for a given `NetworkRequest`. Should only be used
    /// when in mock mode.
    func setMockResponseFor(networkRequest: NetworkRequest, responseConnection: HttpConnection?) {
-        helper.setResponseFor(networkRequest: networkRequest, responseConnection: responseConnection)
+        helper.addResponseFor(networkRequest: networkRequest, responseConnection: responseConnection ?? defaultMockResponse(networkRequest.url))


Removed the defaults here, and updated the MockNetworkService connectAsync to own the default directly

kevinlind

One minor comment about documentation, otherwise looks good.

kevinlind · 2023-09-21T22:28:59Z

Tests/TestUtils/NetworkRequestHelper.swift

-        return networkResponses
-            .filter { networkRequest.isCustomEqual($0.key) }
-            .map { $0.value }
+    /// Gets all network responses for the given `NetworkRequest`


nit: Update comment "Get network response for the given NetworkRequest", plus add comment for parameter and return value.

Updated method docs!

timkimadobe added 10 commits September 20, 2023 14:01

Implement TestableNetworkRequest using @testable import for access to…

825841e

… NetworkRequest Override Equatable and Hashable implementations for use in testing comparisons

Refactor NetworkRequestHelper data properties to use new TestableNetw…

be8c6d5

…orkRequest as keys Refactor usages to use TestableNetworkRequest methods Rename setResponseFor to addResponseFor to more accurately reflect what the method does

Refactor Mock and RealNetworkService based on changes to NetworkServi…

872b7f5

…ceHelper

Refactor UpstreamIntegrationTests based on changes to RealNetworkService

a6744b4

Add convenience init to TestableNetworkRequest and update usages

ac45a00

Update implementation to use lowercased string comparison and hash fo…

f70ed39

…r alignment

Remove unused NetworkService isCustomEquals method

f79104d

Update docs to reflect actual method name

6f8ffab

Align the logical implementations of isEqual and hash

2253c0e

timkimadobe requested review from emdobrin, kevinlind, cacheung and addb September 20, 2023 23:43

kevinlind reviewed Sep 21, 2023

View reviewed changes

timkimadobe added 3 commits September 21, 2023 14:20

Refactor networkResponses data struct to be 1:1 request to response

1cd347f

Refactor related logic for getting and setting, and usage sites

Add method and class docs for TestableNetworkRequest

ad669cc

Remove defaultMockResponse property, and place it inline with only pl…

8fc7331

…ace it is used

timkimadobe commented Sep 21, 2023

View reviewed changes

timkimadobe requested a review from kevinlind September 21, 2023 21:23

kevinlind approved these changes Sep 21, 2023

View reviewed changes

Update method doc for getResponseFor

67bebb0

timkimadobe merged commit 285aeb9 into adobe:dev Sep 21, 2023

timkimadobe mentioned this pull request Sep 22, 2023

NetworkService test helpers parity and method doc updates adobe/aepsdk-edge-android#104

Merged

10 tasks

timkimadobe deleted the testable-import-network-request branch September 29, 2023 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testable NetworkRequest #404

Testable NetworkRequest #404

timkimadobe commented Sep 20, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading

kevinlind Sep 20, 2023

timkimadobe Sep 21, 2023

kevinlind Sep 20, 2023

timkimadobe Sep 21, 2023

kevinlind Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

kevinlind Sep 21, 2023

timkimadobe Sep 21, 2023

kevinlind Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe left a comment

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

timkimadobe Sep 21, 2023

kevinlind left a comment

kevinlind Sep 21, 2023

timkimadobe Sep 21, 2023

Testable NetworkRequest #404

Testable NetworkRequest #404

Conversation

timkimadobe commented Sep 20, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

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

timkimadobe 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

kevinlind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkimadobe commented Sep 20, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading