chore: refactor shared tests for modular testing #494

mrehan27 · 2024-02-01T08:49:43Z

Changes

Updated test dependencies to align with actual dependencies for consistency
Introduced UnitTestBase to facilitate module-wise testing setup
Refactored UnitTest and IntegrationTest classes in SharedTest
Implemented base UnitTest and IntegrationTest classes in DataPipeline, Tracking, MessagingInApp, and MessagingPush to enable independent testing of each module
Streamlined and relocated utility methods for testing to enhance consistency
Updated documentation

github-actions · 2024-02-01T08:49:59Z

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.

CocoaPods-FCM: Build failed. See CI job logs to determine the issue and try re-building.
APN-UIKit: mob-51 (1707142605)

github-actions · 2024-02-01T08:50:16Z

	Warnings
⚠️	Swift Package Manager files (Package.) were modified but Cocoapods files (.podspec) files were not. This is error-prone when updating dependencies in one service but not the other. Double-check that you updated all of the correct files.

Generated by 🚫 dangerJS against 7ea5628

mrehan27 · 2024-02-01T08:59:20Z

Sources/Common/CustomerIOInstance.swift

-     Make testing the singleton `instance` possible.
-     Note: It's recommended to delete app data before doing this to prevent loading persisted credentials
-     */
-    static func resetSharedInstance() {


Moved this and the constructor to #if DEBUG, assuming it isn't required by the customers

mrehan27 · 2024-02-01T10:44:41Z

Sources/DataPipeline/CustomerIO.swift

+     Common initialization method for setting up the shared `CustomerIO` instance.
+     This method is intended to be used by both actual implementations and in tests, ensuring that tests closely mimic the real-world implementation.
+     */
+    private static func initialize(implementation: DataPipelineInstance, diGraph: DIGraph) {


Added this to make sure both tests and actual SDK is initialized using same path.

mrehan27 · 2024-02-01T10:49:19Z

Sources/Common/CustomerIOInstance.swift

+    // Any implementation of the interface works for unit tests.
+
+    @discardableResult
+    static func setUpSharedInstanceForUnitTest(implementation: CustomerIOInstance, diGraph: DIGraph) -> CustomerIO {


All classes have now similar names, I tried to keep them descriptive for convenience.

would we need DiGraphShared here as well?

and we should ideally not need DIGraph at all here, since its not being replaced with DiGraphShared

Added only DIGraph here as CustomerIOInstance only uses it in Common yet. But I do agree, we do need to cleanup graphs. I'll create separate ticket for it.

mrehan27 · 2024-02-01T10:50:43Z

Tests/Shared/Core/UnitTestBase.swift

+/// or a specific module (e.g., `MessagingPush`).
+///
+/// For SDK-wide tests, child classes can conveniently inherit from `UnitTest`, designed specifically for testing only SDK APIs.
+open class UnitTestBase<Component>: XCTestCase {


This is very much similar to what we have today in SharedTests.UnitTest, with only relocation and helping new changes

Sources/DataPipeline/DataPipelineConfigOptions.swift

mrehan27 · 2024-02-01T11:45:19Z

Tests/DataPipeline/Core/IntegrationTest.swift

+        diGraph.override(value: deviceInfoStub, forType: DeviceInfo.self)
+    }
+
+    override open func initializeSDKComponents() -> CustomerIO? {


This is how each module can setup SDK/module initialization the way they want

mrehan27 · 2024-02-01T11:52:05Z

Tests/DataPipeline/Core/UnitTest.swift

+/// Base class for unit testing within the module, extending `UnitTestBase` with setup and utilities
+/// specific to module components. Ideal for isolated tests of individual functions and classes.
+open class UnitTest: SharedTests.UnitTestBase<CustomerIO> {
+    public var dataPipelineConfigOptions: DataPipelineConfigOptions!


Every module can have their own config

mrehan27 · 2024-02-01T11:53:46Z

Tests/Shared/Core/UnitTestBase.swift

+        // setup and override dependencies before creating SDK instance, as Shared graph may be initialized and used immediately
+        setUpDependencies()
+        // setup SDK instance and set necessary components for testing
+        initializeSDKComponents()


Used open methods for setting up SDK/module so they can be overridden and implemented by child classes as needed

mrehan27 · 2024-02-01T11:54:51Z

Tests/Tracking/Core/UnitTest.swift

+
+    // function meant to only be in tests as deleting all files from a search path (where app files can be stored!) is
+    // not a good idea.
+    private func deleteAllFiles() {


Moved this from SharedTest to Tracking as I think it is only needed by Tracking module now

levibostian

Would you mind breaking apart this pull request into smaller ones to facilitate the review process? The size of it is quite large, and it is taking some time to review.

I really appreciate the thorough breakdown of the changes in the pull request description. It seems that the breakdown could provide helpful guidance on how to divide the changes into smaller, more manageable pull requests.

mrehan27 · 2024-02-02T07:33:10Z

Thank @levibostian for sharing the concern. I've moved this discussion to Slack so we can talk about it in more detail and resolve it more quickly.

Shahroz16 · 2024-02-05T13:03:23Z

Package.swift

Should we also update Tracking test target with this?

Not really needed. Because tracking has already correct dependencies (i.e. DataPipeline and SharedTests). And unfortunately, test target of DataPipeline cannot be added as dependency to Tracking so they don't need to be updated.

Shahroz16 · 2024-02-05T13:05:59Z

Sources/Common/CustomerIOInstance.swift

+    // Any implementation of the interface works for unit tests.
+
+    @discardableResult
+    static func setUpSharedInstanceForUnitTest(implementation: CustomerIOInstance, diGraph: DIGraph) -> CustomerIO {


would we need DiGraphShared here as well?

Shahroz16 · 2024-02-05T13:07:23Z

Sources/Common/CustomerIOInstance.swift

+    // Any implementation of the interface works for unit tests.
+
+    @discardableResult
+    static func setUpSharedInstanceForUnitTest(implementation: CustomerIOInstance, diGraph: DIGraph) -> CustomerIO {


and we should ideally not need DIGraph at all here, since its not being replaced with DiGraphShared

Shahroz16 · 2024-02-05T13:10:53Z

Sources/Tracking/CustomerIO.swift

-            // automatically add the AutoTrackingScreenViews plugin
-            DataPipeline.shared.analytics.add(plugin: AutoTrackingScreenViews(filterAutoScreenViewEvents: sdkConfig.filterAutoScreenViewEvents, autoScreenViewBody: sdkConfig.autoScreenViewBody))
-        }
+        initialize(implementation: implementation, diGraph: newDiGraph)


I am guessing there is not change is the code but rather just made a method for the same changes?

Yes, correct. No change in logic, only code moved to new initialize so it can be reused by methods initializing the module for testing.

Shahroz16 · 2024-02-05T13:13:25Z

Sources/Tracking/CustomerIO.swift

+    static func setUpSharedInstanceForIntegrationTest(diGraphShared: DIGraphShared, diGraph: DIGraph, autoAddCustomerIODestination: Bool) -> CustomerIOInstance {
+        let sdkConfig = diGraph.sdkConfig
+        var moduleConfig = DataPipelineConfigOptions.Factory.create(sdkConfig: sdkConfig)
+        // allow overriding autoAddCustomerIODestination so http requests can be ignored during tests


I believe its because in tracking config we do not have any concept of this, let's add a comment explaining that if that's true

Correct, updated

Shahroz16 · 2024-02-05T13:16:49Z

Tests/DataPipeline/Core/UnitTest.swift

+        enableLogs: Bool = false,
+        siteId: String? = nil,
+        writeKey: String? = nil,
+        modifySdkConfig: ((inout SdkConfig) -> Void)? = nil,


we should ideally be moving away from sdkConfig as well from test, maybe in next tickets.

Agreed, I'll create ticket for it.

Shahroz16 · 2024-02-05T13:22:43Z

Sources/Tracking/CustomerIO.swift

+    // Integration tests use the actual implementation.
+
+    @discardableResult
+    static func setUpSharedInstanceForIntegrationTest(diGraphShared: DIGraphShared, diGraph: DIGraph, autoAddCustomerIODestination: Bool) -> CustomerIOInstance {


autoAddCustomerIODestination should probably have a default value?

Shahroz16

Looks good 👍

levibostian

With Shahroz's approval and seeing the good conversation in the PR, I'll remove my request for change to help unblock this.

mrehan27 added 22 commits January 30, 2024 17:06

updated package file

bc73b8d

unit test updates

ffa63d7

more test fixes

a9fc4ae

minor fixes

3f45107

added comments

aa14788

more test fixes

1ff5759

method rename

3e7a8a0

minor fix

3d5a2e8

ignored http requests

286f2c3

format fixes

5e56fcd

more test fixes

d092421

config fix

d9abae1

config rename

653a026

tracking test fixes

25af56d

added generic

548c904

child class updates

4255cc0

MessagingPush fixes

8bfdea0

consistent naming

1324c3b

added test base

79ecdea

doc updates

4509427

more doc updates

6d90915

base test classes for inapp

244b79e

mrehan27 self-assigned this Feb 1, 2024

mrehan27 commented Feb 1, 2024

View reviewed changes

mrehan27 marked this pull request as ready for review February 1, 2024 10:51

mrehan27 requested a review from a team February 1, 2024 10:52

minor fix

eb43eb9

mrehan27 commented Feb 1, 2024

View reviewed changes

levibostian suggested changes Feb 1, 2024

View reviewed changes

doc updates

d155ceb

Shahroz16 reviewed Feb 5, 2024

View reviewed changes

mrehan27 added 2 commits February 5, 2024 19:09

pr suggestions

4f2f81c

caller updates

7ea5628

Shahroz16 approved these changes Feb 5, 2024

View reviewed changes

levibostian reviewed Feb 5, 2024

View reviewed changes

levibostian approved these changes Feb 5, 2024

View reviewed changes

mrehan27 merged commit 2daa7b2 into main-replica-for-cdp Feb 6, 2024
4 of 7 checks passed

mrehan27 deleted the mob-51 branch February 6, 2024 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor shared tests for modular testing #494

chore: refactor shared tests for modular testing #494

mrehan27 commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

Shahroz16 Feb 5, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

mrehan27 Feb 1, 2024

levibostian left a comment

mrehan27 commented Feb 2, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

Shahroz16 Feb 5, 2024

Shahroz16 Feb 5, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

Shahroz16 Feb 5, 2024

mrehan27 Feb 5, 2024

Shahroz16 left a comment

levibostian left a comment

chore: refactor shared tests for modular testing #494

chore: refactor shared tests for modular testing #494

Conversation

mrehan27 commented Feb 1, 2024 • edited Loading

Changes

github-actions bot commented Feb 1, 2024 • edited Loading

Sample app builds 📱

github-actions bot commented Feb 1, 2024 • 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

levibostian left a comment

Choose a reason for hiding this comment

mrehan27 commented Feb 2, 2024

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

Choose a reason for hiding this comment

Shahroz16 left a comment

Choose a reason for hiding this comment

levibostian left a comment

Choose a reason for hiding this comment

mrehan27 commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading