adobe / aepsdk-edge-ios

Adobe Experience Platform Edge Network mobile extension in Swift
Apache License 2.0
13 stars 24 forks source link

Integration test helper parity #403

Closed timkimadobe closed 1 year ago

timkimadobe commented 1 year ago

Description

This PR:

  1. Adds a new TestableNetworkRequest wrapper around NetworkRequest to enable custom conformance to Equatable and Hashable for direct use as a dictionary key.
  2. Refactors the NetworkServiceHelper data structs to use use the new TestableNetworkRequest as the dictionary key directly, and updates value types according to the new grouping behavior
    1. Renames the setResponseFor method to addResponseFor to more accurately reflect what the method does
  3. Refactors the Mock and RealNetworkService based on these changes
    1. In MockNetworkService: creates a new closure that creates a default mock response using a valid URL
  4. Refactors the UpstreamIntegrationTests to use optional chaining on the newly optional network response array

Motivation: This update makes the custom implementation of Equatable and Hashable for testing purposes more explicit, and prevents accidental implementation errors, where the isCustomEquals is not applied correctly in the current implementation state. It also brings the iOS implementation in line with the Android version: https://github.com/adobe/aepsdk-edge-android/blob/main/code/edge/src/androidTest/java/com/adobe/marketing/mobile/services/TestableNetworkRequest.java

Alternative implementations considered: Using extension NetworkRequest and overriding the existing Equatable and Hashable implementations is not possible based on the current minimum supported version of iOS 11.0, as iOS 13.0 is required to allow '@objc' property in extension of subclass

Screenshot 2023-09-19 at 4 31 18 PM

Question for reviewers:

  1. The use of initializing TestableNetworkRequest in if statements and elsewhere seems a bit clumsy, is there a more elegant way to do this?:
    if expectedNetworkRequest.key == TestableNetworkRequest(networkRequest: networkRequest) {
    expectedNetworkRequest.value.countDown()
    }

i. Maybe using an underscored parameter for the networkRequest initializer?:

init(_ networkRequest: NetworkRequest) {
    self.networkRequest = networkRequest
}

// Callsite becomes:
if expectedNetworkRequest.key == TestableNetworkRequest(networkRequest) {
    expectedNetworkRequest.value.countDown()
}

ii. Or using a custom == on TestableNetworkRequest that can accept NetworkRequest as the rhs argument?

  1. Should the test target minimum deployment be updated to iOS 13? It would enable using the alternative implementation of overriding NetworkRequest's Equatable and Hashable implementations directly, but leaves a gap in functional and integration testing for iOS versions 11.0 <-> 12.X (highest up to 13.0)

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov[bot] commented 1 year ago

Codecov Report

Merging #403 (7082bdc) into dev (c3ce8a6) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #403   +/-   ##
=======================================
  Coverage   96.77%   96.77%           
=======================================
  Files          27       27           
  Lines        1671     1671           
=======================================
  Hits         1617     1617           
  Misses         54       54           
timkimadobe commented 1 year ago

Planning on reimplementing using @Testable import AEPServices instead