Refactor unit test cases to use JSON comparison (part 1) by timkimadobe · Pull Request #434 · adobe/aepsdk-edge-ios

timkimadobe · 2023-11-22T22:30:14Z

Note

This is the first part in a multi-part series of PRs that will refactor existing Edge extension unit and functional tests to use the JSON comparison tool from AEPTestUtils
See part 2: #436

Description

This PR refactors some of the Edge extension unit tests to use JSON comparison

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.

…dsNetworkRequest_returnsTrue testProcessHit_experienceEvent_withDatastreamOverrideSet_sendsNetworkRequest_returnsTrue change intent: 1. Refactor existing mock network service method usage to AEPTestUtils MockNetworkService, which explicitly requires URL a. Has the downstream effect of more strictly validating the request 2. Take advantage of new functionality in mockNetworkService.assertAllNetworkRequestExpectations to validate both: a. Expected events b. No unexpected events (toggleable) 3. Remove mock response from test case setup since it is the same as the mock response set in helper assertProcessHit

…urnsTrue Remove mock response from test case setup since it is the same as the mock response set in helper assertProcessHit

To take advantage of new functionality in mockNetworkService.assertAllNetworkRequestExpectations to validate: a. Expected events b. No unexpected events (toggleable) c. No requests sent out (no expectations + no unexpected events allowed)

codecov · 2023-11-22T22:40:30Z

Codecov Report

Merging #434 (f8814bd) into dev (74a67e5) will decrease coverage by 0.29%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #434      +/-   ##
==========================================
- Coverage   96.76%   96.47%   -0.29%     
==========================================
  Files          27       27              
  Lines        1730     1730              
==========================================
- Hits         1674     1669       -5     
- Misses         56       61       +5

kevinlind · 2023-11-30T00:28:45Z

Tests/UnitTests/EdgeHitProcessorTests.swift

+        {
+          "xdm": {
+            "implementationDetails": null
+          }
+        }


Does this mean that "implementationDetails" is not added to the "xdm" object, or that the field "implementationDetails" is added but has a value of "null"? I would expect "implementationDetails" to not be included in the request.

UPDATE: Validating a key is not present is now possible with the changes in: adobe/aepsdk-testutils-ios#15

See example mocked test failure when keys are present:

Outdated:
The way the validation works currently by default, putting: "implementationDetails": null in the expected JSON means that either case on the actual side causes validation to pass:

implementationDetails key exists and its value must be nil

implementationDetails key does not exist, however an implicit requirement of the comparison system is that the parent collection has at least the same number of children elements as fields you're validating are null

In this case, xdm on the actual side has the child element: "identityMap": null meaning that setting "implementationDetails": null on the expected side doesnt cause collection size validation to fail

If implementation were to change and "identityMap": null was not present on the actual side, this test case would start failing, even though "identityMap": null is not being directly tested

That is, the comparison system describes positive validation well (the presence of keys and their values) but negative validation (the absence of keys) is a bit roundabout

Hopefully my explanation makes sense, please let me know what you think!

If this implicit requirement isn't desirable, some potential alternatives are:

Update the comparison system to add a new mode which allows for validating a key is not present (not equal to nil or implicitly nil by not being present as it is today)

Building on the changes in: Refactor JSON comparison options handling aepsdk-testutils-ios#9

Refactor the validation to use positive validation of what keys are expected and strictly validate the collection size

kevinlind · 2023-11-30T00:29:38Z

Tests/UnitTests/EdgeHitProcessorTests.swift

        XCTAssertNil(payload["xdm.implementationDetails.name"])
        XCTAssertNil(payload["xdm.implementationDetails.version"])
        XCTAssertNil(payload["xdm.implementationDetails.environment"])


Are these tests still needed if you are validating the entire JSON request below?

No, they're not needed - sorry I forgot to remove these lines! Thanks Kevin

kevinlind · 2023-11-30T00:31:53Z

Tests/UnitTests/EdgeNetworkServiceTests.swift

+            // verify
+            let networkRequests = self.mockNetworkService.getNetworkRequestsWith(url: self.url, httpMethod: .post)
+            guard let networkRequest = networkRequests.first else {
+                XCTFail("Unable to find valid network request.")
+                return
+            }
+
+            XCTAssertTrue(success)
+            XCTAssertNil(retryInterval)
+            XCTAssertEqual(1, networkRequests.count)
+            XCTAssertEqual(testHeaders.count + self.defaultNetworkingHeaders.count, networkRequest.httpHeaders.count)
+            for header in testHeaders {
+                XCTAssertNotNil(networkRequest.httpHeaders[header.key])
+            }
+
+            for header in self.defaultNetworkingHeaders {
+                XCTAssertNotNil(networkRequest.httpHeaders[header])
+            }
+            expectation.fulfill()
+        })


Can you double check the indentation here? You may need to convert tabs to spaces if you are using them.

Were you referring to the indent being one level higher for the verify section? I think it might be because it's inside the doRequest closure. I checked the whitespace characters and I believe it's using spaces:

The previous code was indented from the "completion" block but now it appears to be indented from the "networkService.doRequest" block. If you look at the next few test cases which call doRequest you can see the completion closure code is indented under the "completion" parameter name. As long as you've run SwiftLint to autocorrect the formatting, then I think this is fine. I was just confused as to why the indentation changed.

timkimadobe

Thanks for the review @kevinlind! Updated based on feedback

timkimadobe · 2023-11-30T00:50:51Z

Tests/UnitTests/EdgeNetworkServiceTests.swift

+            // verify
+            let networkRequests = self.mockNetworkService.getNetworkRequestsWith(url: self.url, httpMethod: .post)
+            guard let networkRequest = networkRequests.first else {
+                XCTFail("Unable to find valid network request.")
+                return
+            }
+
+            XCTAssertTrue(success)
+            XCTAssertNil(retryInterval)
+            XCTAssertEqual(1, networkRequests.count)
+            XCTAssertEqual(testHeaders.count + self.defaultNetworkingHeaders.count, networkRequest.httpHeaders.count)
+            for header in testHeaders {
+                XCTAssertNotNil(networkRequest.httpHeaders[header.key])
+            }
+
+            for header in self.defaultNetworkingHeaders {
+                XCTAssertNotNil(networkRequest.httpHeaders[header])
+            }
+            expectation.fulfill()
+        })


Were you referring to the indent being one level higher for the verify section? I think it might be because it's inside the doRequest closure. I checked the whitespace characters and I believe it's using spaces:

timkimadobe · 2023-11-30T01:15:33Z

Tests/UnitTests/EdgeHitProcessorTests.swift

+        {
+          "xdm": {
+            "implementationDetails": null
+          }
+        }


UPDATE: Validating a key is not present is now possible with the changes in: adobe/aepsdk-testutils-ios#15

See example mocked test failure when keys are present:

Outdated:
The way the validation works currently by default, putting: "implementationDetails": null in the expected JSON means that either case on the actual side causes validation to pass:

implementationDetails key exists and its value must be nil

implementationDetails key does not exist, however an implicit requirement of the comparison system is that the parent collection has at least the same number of children elements as fields you're validating are null

In this case, xdm on the actual side has the child element: "identityMap": null meaning that setting "implementationDetails": null on the expected side doesnt cause collection size validation to fail

If implementation were to change and "identityMap": null was not present on the actual side, this test case would start failing, even though "identityMap": null is not being directly tested

That is, the comparison system describes positive validation well (the presence of keys and their values) but negative validation (the absence of keys) is a bit roundabout

Hopefully my explanation makes sense, please let me know what you think!

If this implicit requirement isn't desirable, some potential alternatives are:

Update the comparison system to add a new mode which allows for validating a key is not present (not equal to nil or implicitly nil by not being present as it is today)

Building on the changes in: Refactor JSON comparison options handling aepsdk-testutils-ios#9

Refactor the validation to use positive validation of what keys are expected and strictly validate the collection size

timkimadobe · 2023-11-30T01:17:07Z

Tests/UnitTests/EdgeHitProcessorTests.swift

        XCTAssertNil(payload["xdm.implementationDetails.name"])
        XCTAssertNil(payload["xdm.implementationDetails.version"])
        XCTAssertNil(payload["xdm.implementationDetails.environment"])


No, they're not needed - sorry I forgot to remove these lines! Thanks Kevin

cdhoffmann · 2023-11-30T23:44:30Z

Tests/UnitTests/EdgeEventErrorTests.swift

+        }
+        """#
+
+        assertEqual(expected: getAnyCodable(expectedJSON)!,


This is more of an idea around the assertEqual method. What do you think about overloading the method to take the raw json instead, and internally doing the anycodable conversions? Will make the API easier to use in our tests. Not sure if it should just replace this method, or if it should be overloaded in case we want to support both.

I think this is a great idea! I feel like there is quite a bit of boilerplate we can remove from the method callsite. I've implemented a new protocol that handles type conversion to AnyCodable here:
adobe/aepsdk-testutils-ios#16

kevinlind

Looks good, though you'll need to fix the build failures before merging.

timkimadobe added 12 commits November 21, 2023 14:33

Update AEPTestUtils branch

d8f87b6

Refactor EdgeEventErrorTests to use JSON comparison

25dcd32

Refactor EdgeEventWarningTests to use JSON comparison

a154d26

Remove old assertion logic

63e9b26

Refactor EdgeExtensionTests to use JSON comparison

cd18325

Refactor EdgeHitProcessorTests to use AEPTestUtils MockNetworkService

f7ef46e

Refactor testProcessHit_experienceEvent_happy_sendsNetworkRequest_ret…

2ffc7b4

…urnsTrue Remove mock response from test case setup since it is the same as the mock response set in helper assertProcessHit

Update AEPTestUtils branch dependency

25d2fd2

To take advantage of new functionality in mockNetworkService.assertAllNetworkRequestExpectations to validate: a. Expected events b. No unexpected events (toggleable) c. No requests sent out (no expectations + no unexpected events allowed)

Update expected event count to be derived from recoverable error count

c185b0d

Refactor EdgeHitProcessorTests

2b1be43

Refactor EdgeHitTests to use JSON comparison

f818eda

timkimadobe requested review from addb, cacheung and kevinlind November 22, 2023 22:30

timkimadobe added 3 commits November 22, 2023 15:38

Get latest AEPTestUtils

2d237ec

Refactor EdgeNetworkServiceTests to use AEPTestUtils

e67faae

Apply lint autocorrect

f66d33f

timkimadobe mentioned this pull request Nov 23, 2023

Refactor unit test cases to use JSON comparison (part 2) #435

Closed

10 tasks

timkimadobe requested review from cdhoffmann and emdobrin November 29, 2023 23:29

timkimadobe linked an issue Nov 29, 2023 that may be closed by this pull request

Refactor functional and integration tests to use new test utils #430

Closed

kevinlind reviewed Nov 30, 2023

View reviewed changes

Remove redundant validation

2bf728d

timkimadobe commented Nov 30, 2023

View reviewed changes

timkimadobe requested a review from kevinlind November 30, 2023 01:24

cdhoffmann approved these changes Nov 30, 2023

View reviewed changes

timkimadobe added 2 commits December 14, 2023 20:06

Update AEPTestUtils to point to feature/latest

c22aa66

Refactor unit tests to use latest AEPTestUtils updates

ed15f23

timkimadobe mentioned this pull request Dec 15, 2023

Refactor unit test cases to use JSON comparison (part 2) #436

Merged

10 tasks

kevinlind approved these changes Jan 6, 2024

View reviewed changes

timkimadobe added 3 commits January 8, 2024 14:30

Update pods

3952ef7

Fix build errors - UpstreamIntegrationTests

b3ecd09

Fix build errors - FunctionalTests

f8814bd

timkimadobe merged commit 2c9403f into adobe:dev Jan 8, 2024

timkimadobe deleted the refactor-for-json-comparison branch January 8, 2024 22:42

Conversation

timkimadobe commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdhoffmann Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinlind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timkimadobe commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading

timkimadobe Nov 30, 2023 •

edited

Loading

timkimadobe Nov 30, 2023 •

edited

Loading

cdhoffmann Nov 30, 2023 •

edited

Loading

timkimadobe Dec 12, 2023 •

edited

Loading