test: split crash reporter tests into separate target #2899

armcknight · 2023-04-13T18:43:07Z

reimplement some ojbc classes needed to move into the shared SentryTestUtils lib in swift since you cant mix them in a static library
reimplement some swift classes into objc since they were used from other objc files and i couldn't get the "-Swift.h" importation working
fixed some test assertions that started failing after making changes, not sure how they ever worked in the first place
fixed a bug with async frames in stack trace builder, also not sure how the associated test was passing

TODO:

figure out why SentryCrashInstallationTests.testUninstall` is failing, need some logging because it passes locally
give the tests a little more time to run, the Xcode 13 variants are timing out

#skip-changelog

…essary ojbc classes into swift since you cant mix them in a static library fails to build because it looks like a c++ source is being included into the bridging header; or, the bridging header is being used incorrectly by the new target

…bridging header to tests bridging header and reimplement url delegate spy in objc because couldnt import the swift implementation into one of the objc test classes

…ate-crash-test-target

armcknight · 2023-04-13T18:55:59Z

Tests/SentryTests/Helper/UrlSessionDelegateSpy.swift

@@ -1,17 +0,0 @@
-import Foundation
-
-class UrlSessionDelegateSpy: NSObject, URLSessionDelegate {


rewrote this in objc because it needed to be used in an objc test class and i couldn't get the "-Swift.h" import to work

It's such a small class so it should be fine.

armcknight · 2023-04-13T18:57:11Z

Tests/SentryTests/SentryCrash/TestSentryUIDeviceWrapper.swift

@@ -1,26 +0,0 @@
-import Sentry
-
-class TestSentryUIDeviceWrapper: SentryUIDeviceWrapper {


This one was simply moved/renamed like so many others into SentryTestUtils, but didn't diff that way

armcknight · 2023-04-13T18:58:11Z

Tests/SentryTests/SentryCrash/SentryCrashInstallationTests.m

-#import "SentryCrashMonitor_MachException.h"
-#import "SentryCrashSystemCapabilities.h"
-#import "SentryNSNotificationCenterWrapper.h"
-#import "SentryTests-Swift.h"


For some reason this generated swift header import stopped working, and i would've also needed one for the SentryTestUtils lib which also couldn't get working, so i just reimplemented this test class in Swift instead.

armcknight · 2023-04-13T19:02:02Z

Tests/SentryTests/SentryCrash/TestSentryCrashWrapper.h

-#import "SentryDefines.h"
-#import <Foundation/Foundation.h>
-
-NS_ASSUME_NONNULL_BEGIN
-
-/**
- * Written in Objective-C because Swift doesn't allow you to call the constructor of
- * TestSentryCrashAdapter. We override sharedInstance in the implementation file to make it work.
- */
-@interface TestSentryCrashWrapper : SentryCrashWrapper
-SENTRY_NO_INIT


Since you cant mix objc and swift in a static lib, and i wanted to move this class into sentrytestutils because now it's used in both the main sdk test target and the new crash test target, i rewrote it in swift.

I also just removed the SENTRY_NO_INIT declaration from SentryCrashWrapper. Since it is passed around in the singleton dependency container, there's no reason to also have it be a singleton, so I figured it'll be fine to allow it to be alloc init'd. We shouldn't have much opportunity to need to accidentally do that in new places vs using the one vended by the dependency container. I converted all the calls to this singleton to use the one vended by the dependency container singleton.

armcknight · 2023-04-13T19:08:02Z

Tests/SentryTests/SentryOptionsTest.m

 #import "SentrySDK.h"
-#import "SentryTests-Swift.h"


This stopped working for some reason, which is why i had to rewrite UrlSessionDelegateSpy in objc.

Thanks for the info. We can fix it in another PR.

armcknight · 2023-04-13T19:15:54Z

Tests/SentryCrashTests/SentryCrashInstallationTests.swift

+    }
+}
+
+final class SentryCrashInstallationTests: XCTestCase {


Rewritten in swift, see my other comment on the old objc implementation that was removed.

armcknight · 2023-04-13T19:16:37Z

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitorType.h

@@ -92,8 +94,6 @@ typedef enum {
 #define SentryCrashMonitorTypeRequired                                                             \
    (SentryCrashMonitorTypeSystem | SentryCrashMonitorTypeApplicationState)

-#define SentryCrashMonitorTypeNone 0


This wouldn't work in the new swift implementation of an objc impl that was using this, so i changed it to be a member of the enum instead.

Sources/Sentry/SentryStacktraceBuilder.m

armcknight · 2023-04-13T19:17:53Z

SentryTestUtils/TestSentryUIDeviceWrapper.swift

@@ -0,0 +1,26 @@
+import Sentry
+
+public class TestSentryUIDeviceWrapper: SentryUIDeviceWrapper {


This wasn't rewritten, it was only renamed/moved, but didn't diff correctly. No changes in this file.

armcknight · 2023-04-13T19:19:21Z

SentryTestUtils/TestCrashWrapper.swift

+import Foundation
+import Sentry
+
+public class TestSentryCrashWrapper: SentryCrashWrapper {


Rewritten in swift, see later comment on the old objc implementation.

brustolin · 2023-04-14T08:09:03Z

fixed a bug with async frames in stack trace builder, also not sure how the associated test was passing

This was fixed at #2898

philipphofmann · 2023-04-14T11:38:05Z

I'm sorry I didn't get to that PR yet. I will give you a review on Monday.

philipphofmann

Thanks @armcknight for this great refactoring. We need to call all test targets in CI. It think it makes sense to run the test targets subsequentially for each matrix config for the unit tests

sentry-cocoa/.github/workflows/test.yml

Lines 86 to 90 in 7419285

    
           - runs-on: macos-11 
        
             platform: 'iOS' 
        
             xcode: '13.2.1' 
        
             test-destination-os: '13.7' 
        
             timeout-minutes: 15

otherwise, we end up with many more GH jobs. Maybe it makes sense to create multiple raw log artifacts per test target. We also need to ensure that the DerivedData Logs still work properly, and that the code coverage is not messed up.

FYI, I'm going to remove this piece of bash script as it's not required anymore this PR #2918

sentry-cocoa/scripts/xcode-test.sh

Lines 61 to 69 in 7419285

    
           if [ $PLATFORM == "iOS" -a $OS == "12.4" ]; then 
        
               # Skip some tests that fail on iOS 12.4. 
        
               env NSUnbufferedIO=YES xcodebuild -workspace Sentry.xcworkspace \ 
        
                   -scheme Sentry -configuration $CONFIGURATION \ 
        
                   -destination "$DESTINATION" \ 
        
                   -skip-testing:"SentryTests/SentrySDKTests/testMemoryFootprintOfAddingBreadcrumbs" \ 
        
                   -skip-testing:"SentryTests/SentrySDKTests/testMemoryFootprintOfTransactions" \ 
        
                   test | tee raw-test-output.log | $RUBY_ENV_ARGS xcpretty -t && slather coverage --configuration $CONFIGURATION && exit ${PIPESTATUS[0]} 
        
           else

philipphofmann · 2023-04-17T14:17:49Z

Sources/Sentry/SentryCrashWrapper.m

@@ -13,14 +13,6 @@

 @implementation SentryCrashWrapper

-+ (instancetype)sharedInstance


m: I think this change could be in an extra PR as it makes sense without all the other changes. Even if we agree not to proceed with this PR, this change would make it to the main branch.

Sources/Sentry/SentryStacktraceBuilder.m

philipphofmann · 2023-04-17T14:20:29Z

Tests/SentryCrashTests/SentryCrashReportFilter_Tests.m

@@ -155,7 +155,6 @@ - (void)testPipelineInit
    id<SentryCrashReportFilter> filter = [[SentryCrashReportFilterPipeline alloc]
        initWithFilters:[SentryCrashReportFilterPassthrough filter],
        [SentryCrashReportFilterPassthrough filter], nil];
-    filter = filter;


Thanks for the comment 😃

philipphofmann · 2023-04-17T14:22:34Z

Tests/SentryTests/Helper/UrlSessionDelegateSpy.swift

@@ -1,17 +0,0 @@
-import Foundation
-
-class UrlSessionDelegateSpy: NSObject, URLSessionDelegate {


It's such a small class so it should be fine.

philipphofmann · 2023-04-17T14:23:32Z

Tests/SentryTests/Integrations/SentryCrash/SentryCrashIntegrationTests.swift

-        let sut = fixture.getSut(crashWrapper: SentryCrashWrapper.sharedInstance())
+        let sut = fixture.getSut(crashWrapper: SentryCrashWrapper()) // ???: should this be using TestSentryCrashWrapper?


Yes, that could make sense. If the test succeeds with TestSentryCrashWrapper, please use it.

philipphofmann · 2023-04-17T14:24:14Z

Tests/SentryTests/SentryOptionsTest.m

 #import "SentrySDK.h"
-#import "SentryTests-Swift.h"


Thanks for the info. We can fix it in another PR.

philipphofmann · 2023-04-17T14:26:57Z

Tests/SentryCrashTests/SentryFrameRemoverTests.swift

+
+private extension SentryFrameRemoverTests {
+    /// Compare elements pairwise to see if the two input arrays are equivalent or not.
+    func equivalent(expected: [Frame], actual: [Frame]) -> Bool {


What you did is okay. I also don't understand why the tests worked before.

philipphofmann · 2023-04-17T14:28:27Z

Tests/SentryCrashTests/SentryFrameRemoverTests.swift

+        for i in 0..<expected.count {
+            let nextExpected = expected[i]
+            let nextActual = actual[i]
+            if nextExpected.package != nextActual.package {


m: I guess we shouldn't only just compare the package only. You could use:

sentry-cocoa/Tests/SentryTests/Protocol/SentryThreadEquality.swift

Lines 35 to 36 in 7419285

extension Sentry.Frame {

open override func isEqual(_ object: Any?) -> Bool {

brustolin · 2023-05-10T12:05:19Z

@armcknight As discussed during a meeting. Can we close this PR?

armcknight added 6 commits April 11, 2023 17:45

fix remaining build issues; had to move some imports from test utils …

88b518e

…bridging header to tests bridging header and reimplement url delegate spy in objc because couldnt import the swift implementation into one of the objc test classes

add crash tests to main scheme

0b2406a

wip: fixing broken tests

111c7fa

fix how arrays were compared for equality; cant test equal objects

2bf3b1c

fix stack trace builder bug with async frames

1738d4e

armcknight requested review from philipphofmann and brustolin as code owners April 13, 2023 18:43

Merge remote-tracking branch 'origin/main' into armcknight/test/separ…

3d328e3

…ate-crash-test-target

armcknight commented Apr 13, 2023

View reviewed changes

armcknight added 2 commits April 13, 2023 11:20

fix bad merge

af6a0f8

fix comparison

9d9f665

philipphofmann reviewed Apr 17, 2023

View reviewed changes

armcknight mentioned this pull request Apr 21, 2023

ref: convert SentryCrashWrapper usage from singleton to dependency injected #2942

Closed

brustolin mentioned this pull request Apr 26, 2023

Extra test target for SentryCrash tests #2880

Closed

armcknight closed this May 11, 2023

armcknight deleted the armcknight/test/separate-crash-test-target branch May 11, 2023 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: split crash reporter tests into separate target #2899

test: split crash reporter tests into separate target #2899

armcknight commented Apr 13, 2023 •

edited

Loading

armcknight Apr 13, 2023

philipphofmann Apr 17, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

philipphofmann Apr 17, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

armcknight Apr 13, 2023

brustolin commented Apr 14, 2023

philipphofmann commented Apr 14, 2023

philipphofmann left a comment •

edited

Loading

philipphofmann Apr 17, 2023

armcknight Apr 21, 2023

philipphofmann Apr 17, 2023

philipphofmann Apr 17, 2023

philipphofmann Apr 17, 2023

philipphofmann Apr 17, 2023

philipphofmann Apr 17, 2023

philipphofmann Apr 17, 2023

brustolin commented May 10, 2023

		@@ -1,17 +0,0 @@
		import Foundation

		class UrlSessionDelegateSpy: NSObject, URLSessionDelegate {

		@@ -1,26 +0,0 @@
		import Sentry

		class TestSentryUIDeviceWrapper: SentryUIDeviceWrapper {

		@@ -0,0 +1,26 @@
		import Sentry

		public class TestSentryUIDeviceWrapper: SentryUIDeviceWrapper {

	- runs-on: macos-11
	platform: 'iOS'
	xcode: '13.2.1'
	test-destination-os: '13.7'
	timeout-minutes: 15

	if [ $PLATFORM == "iOS" -a $OS == "12.4" ]; then
	# Skip some tests that fail on iOS 12.4.
	env NSUnbufferedIO=YES xcodebuild -workspace Sentry.xcworkspace \
	-scheme Sentry -configuration $CONFIGURATION \
	-destination "$DESTINATION" \
	-skip-testing:"SentryTests/SentrySDKTests/testMemoryFootprintOfAddingBreadcrumbs" \
	-skip-testing:"SentryTests/SentrySDKTests/testMemoryFootprintOfTransactions" \
	test \| tee raw-test-output.log \| $RUBY_ENV_ARGS xcpretty -t && slather coverage --configuration $CONFIGURATION && exit ${PIPESTATUS[0]}
	else

		@@ -13,14 +13,6 @@

		@implementation SentryCrashWrapper

		+ (instancetype)sharedInstance

		let sut = fixture.getSut(crashWrapper: SentryCrashWrapper.sharedInstance())
		let sut = fixture.getSut(crashWrapper: SentryCrashWrapper()) // ???: should this be using TestSentryCrashWrapper?

	extension Sentry.Frame {
	open override func isEqual(_ object: Any?) -> Bool {

test: split crash reporter tests into separate target #2899

test: split crash reporter tests into separate target #2899

Conversation

armcknight commented Apr 13, 2023 • 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

brustolin commented Apr 14, 2023

philipphofmann commented Apr 14, 2023

philipphofmann left a comment • 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

brustolin commented May 10, 2023

armcknight commented Apr 13, 2023 •

edited

Loading

philipphofmann left a comment •

edited

Loading