RUMM-1649 WebViewTracking integration test added #714

buranmert · 2022-01-11T13:55:52Z

What and why?

Adding integration test to WebView tracking feature.

How?

In this new test, the app opens a WebView which loads Browser SDK instrumented content. The iOS SDK should transport all RUM events and Logs received from Browser SDK.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference

buranmert · 2022-01-14T15:58:34Z

Most of the changes coming from the merge from master in order to get the new source property

WebViewScenarioTest uses an actual website to test WebView Tracking feature

ncreated · 2022-01-17T08:29:09Z

Most of the changes coming from the merge from master in order to get the new source property

I isolated updating hybrid-application with master to #722 to keep this PR minimal.

…ng test plan this is done by switching crash reporting `.xctestplan` to not include new tests automatically and removing Webviews test from the existing list.

as this is now possible for Browser SDK views.

ncreated · 2022-01-17T11:22:40Z

Datadog/TargetSupport/DatadogIntegrationTests/DatadogCrashReportingIntegrationTests.xctestplan

-      "skippedTests" : [
-        "IntegrationTests",
-        "LoggingScenarioTests",
-        "RUMManualInstrumentationScenarioTests",
-        "RUMMobileVitalsScenarioTests",
-        "RUMModalViewsScenarioTests",
-        "RUMNavigationControllerScenarioTests",
-        "RUMResourcesScenarioTests",
-        "RUMScrubbingScenarioTests",
-        "RUMSwiftUIScenarioTests",
-        "RUMTabBarControllerScenarioTests",
-        "RUMTapActionScenarioTests",
-        "TracingManualInstrumentationScenarioTests",
-        "TracingURLSessionScenarioTests",
-        "TrackingConsentScenarioTests"
+      "selectedTests" : [
+        "CrashReportingWithLoggingScenarioTests\/testCrashReportingCollectOrSendWithLoggingScenario()",
+        "CrashReportingWithRUMScenarioTests\/testCrashReportingCollectOrSendWithRUMScenario()"


This change is on purpose. By default, all new integration tests are added to both XCTest plans (integration and crash reporting), meaning we will run it twice on CI. To avoid this, I disabled "add new tests automatically" in crash reporting XCTest plan. Apparently, this means changing from opt-out to opt-in in .xctestplan 👍.

sent from Browser SDK.

ncreated · 2022-01-17T11:30:02Z

I added more assertions on data received from Browser SDK, this includes checks like:

if Browser events are sent to native iOS RUM session (using native application_id and session_id);
if Browser logs are transported through iOS SDK;
if Browser logs are correlated with native iOS RUM session.

maxep · 2022-01-18T08:26:11Z

Tests/DatadogTests/Matchers/RUMEventMatcher.swift

+        if let eventType = json["type"] as? String,
+           eventType == "resource",
+           var resource = json["resource"] as? [String: Any],
+           let method = resource["method"] as? String {
+            resource["method"] = method.uppercased()
+            json["resource"] = resource
+        }


Is it possible to do case-insensitive comparison instead of transforming the payload?

Not if we want to keep using RUMEventMatcher. This matcher decodes certain RUM event model from Data:

func model<DM: Decodable>(file: StaticString = #file, line: UInt = #line) throws -> DM

RUM events schema defines uppercased values for resource.method:

"method": { "type": "string", "description": "HTTP method of the resource", "enum": ["POST", "GET", "HEAD", "PUT", "DELETE", "PATCH"], "readOnly": true },

Because our RUMDataModels definition is strict about this and Browser SDK seems not, receiving lowercased method name from WebView (e.g. "get" instead of "GET") leads to decoding error. IMO this is an inconsistency in Browser SDK and we need to patch it in order to leverage strict validation with RUMEventMatcher and RUMSessionMatcher in our tests.

However, I think we can move this patch to a better place. Residing here in RUMEventMatcher, it is applied globally to all our test data, but the only problematic test is WebViewScenarioTest.swift where we consume real Browser SDK events. I'd vote for transforming data directly in WebViewScenarioTest.swift before passing it to the matcher, as this will minimalize the impact of this patch and be more explanatory. WDYT on this @maxep ?

Alternatively, we could resign from using strict matchers and assert all values as [String: Any] on events deserialized with JSONSerialization. This way we can perform case-insensitive comparison, but we lose all other strict validation from RUMSessionMatcher. Adding this validation back would require bloating WebViewScenarioTest.swift with manual deserialization code and duplication of all validation concepts.

Yes, I agree! it would be better to move it where the transformation is needed. It clarify that we relax the rule for this specific use case only.

Moved to WebViewScenarioTest.swift ✅. I also talked with browser-sdk team - they will fix it in future release.

Such fix is required, as some queries in hybrid apps (e.g. count all resources by resource.method) might be giving odd results (i.e. considering "post" and "POST" as separate methods).

…ot all events within our tests.

maxep

👌

buranmert requested a review from a team as a code owner January 11, 2022 13:55

buranmert self-assigned this Jan 11, 2022

buranmert force-pushed the buranmert/RUMM-1649-integration-tests branch from 20b2f5d to 27b27ea Compare January 12, 2022 11:09

buranmert marked this pull request as draft January 13, 2022 13:48

RUMM-1649 WebViewTracking integration test added

347ea1a

buranmert force-pushed the buranmert/RUMM-1649-integration-tests branch from 27b27ea to 85b4528 Compare January 14, 2022 15:56

buranmert marked this pull request as ready for review January 14, 2022 15:57

buranmert force-pushed the buranmert/RUMM-1649-integration-tests branch from 85b4528 to 048ee6b Compare January 14, 2022 16:08

RUMM-1649 PR comments addressed

1d95970

WebViewScenarioTest uses an actual website to test WebView Tracking feature

buranmert force-pushed the buranmert/RUMM-1649-integration-tests branch from 048ee6b to 1d95970 Compare January 14, 2022 16:39

ncreated assigned ncreated and unassigned buranmert Jan 17, 2022

ncreated changed the base branch from feature/hybrid-application to ncreated/RUMM-1649-update-hybrid-apps-with-master January 17, 2022 08:28

RUMM-1649 Disable Webviews tracking integration test in Crash Reporti…

da0d544

…ng test plan this is done by switching crash reporting `.xctestplan` to not include new tests automatically and removing Webviews test from the existing list.

ncreated marked this pull request as draft January 17, 2022 08:53

Base automatically changed from ncreated/RUMM-1649-update-hybrid-apps-with-master to feature/hybrid-application January 17, 2022 10:15

ncreated added 2 commits January 17, 2022 12:17

RUMM-1649 Allow viewVisit.name to be nil in RUMSessionMatcher

10fabe3

as this is now possible for Browser SDK views.

RUMM-1649 Remove obsolete TODO

dda0851

ncreated reviewed Jan 17, 2022

View reviewed changes

RUMM-1649 Add assertions to more properties and to logs

6c260e5

sent from Browser SDK.

ncreated force-pushed the buranmert/RUMM-1649-integration-tests branch from 85da275 to 6c260e5 Compare January 17, 2022 11:27

ncreated marked this pull request as ready for review January 17, 2022 11:30

ncreated requested a review from maxep January 17, 2022 11:30

maxep reviewed Jan 18, 2022

View reviewed changes

RUMM-1649 Apply resource.method patch only to Browser SDK events, n…

1593ae7

…ot all events within our tests.

maxep approved these changes Jan 18, 2022

View reviewed changes

ncreated merged commit 8636d3b into feature/hybrid-application Jan 18, 2022

ncreated deleted the buranmert/RUMM-1649-integration-tests branch January 18, 2022 14:34

ncreated mentioned this pull request Jan 19, 2022

RUMM-1911 Combined merge of WebView tracking #729

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-1649 WebViewTracking integration test added #714

RUMM-1649 WebViewTracking integration test added #714

buranmert commented Jan 11, 2022 •

edited by ncreated

Loading

buranmert commented Jan 14, 2022

ncreated commented Jan 17, 2022

ncreated Jan 17, 2022

ncreated commented Jan 17, 2022

maxep Jan 18, 2022

ncreated Jan 18, 2022

maxep Jan 18, 2022

ncreated Jan 18, 2022

maxep left a comment

RUMM-1649 WebViewTracking integration test added #714

RUMM-1649 WebViewTracking integration test added #714

Conversation

buranmert commented Jan 11, 2022 • edited by ncreated Loading

What and why?

How?

Review checklist

buranmert commented Jan 14, 2022

ncreated commented Jan 17, 2022

ncreated Jan 17, 2022

Choose a reason for hiding this comment

ncreated commented Jan 17, 2022

maxep Jan 18, 2022

Choose a reason for hiding this comment

ncreated Jan 18, 2022

Choose a reason for hiding this comment

maxep Jan 18, 2022

Choose a reason for hiding this comment

ncreated Jan 18, 2022

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

buranmert commented Jan 11, 2022 •

edited by ncreated

Loading