Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Fix asserts for SentryCrashTestInstallation #2500

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

philipphofmann
Copy link
Member

Two asserts wrapped with SentryCrashCRASH_HAS_UIAPPLICATION were not executed. This is fixed now.

Came up in #2440 (comment)

#skip-changelog

Two asserts wrapped with SentryCrashCRASH_HAS_UIAPPLICATION were not executed.
This is fixed now.
@@ -62,7 +63,7 @@ - (void)testUninstall_CallsRemoveObservers
[installation uninstall];

#if SentryCrashCRASH_HAS_UIAPPLICATION
XCTAssertEqual(5, self.notificationCenter.removeObserverWithNameInvocations.count);
XCTAssertEqual(5, self.notificationCenter.removeObserverWithNameInvocationsCount);
Copy link
Contributor

@kevinrenskers kevinrenskers Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I woud not add that new property and just keep using removeObserverWithNameInvocations.count.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work as Invocations is not public to ObjC.

@@ -95,7 +96,7 @@ - (void)testUninstall_Install
crashHandlerDataAfterInstall:crashHandlerDataAfterInstall];

#if SentryCrashCRASH_HAS_UIAPPLICATION
XCTAssertEqual(55, self.notificationCenter.removeObserverWithNameInvocations.count);
XCTAssertEqual(55, self.notificationCenter.removeObserverWithNameInvocationsCount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2500 (5219775) into 8.0.0 (a064b3e) will increase coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2500      +/-   ##
==========================================
+ Coverage   78.38%   78.43%   +0.05%     
==========================================
  Files         242      242              
  Lines       22273    22283      +10     
  Branches     9828     9832       +4     
==========================================
+ Hits        17459    17478      +19     
+ Misses       4361     4352       -9     
  Partials      453      453              
Impacted Files Coverage Δ
Sources/Sentry/SentryRequestOperation.m 67.56% <0.00%> (-10.82%) ⬇️
Sources/Sentry/SentryNetworkTracker.m 95.36% <0.00%> (+0.15%) ⬆️
Sources/SentryCrash/Recording/Tools/SentryHook.c 83.82% <0.00%> (+2.94%) ⬆️
Sources/SentryCrash/Recording/Tools/fishhook.c 82.27% <0.00%> (+5.06%) ⬆️
Sources/Sentry/SentryTime.mm 63.15% <0.00%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a064b3e...5219775. Read the comment docs.

@@ -1,8 +1,8 @@
import Foundation

@objc public class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper {
@objcMembers public class TestNSNotificationCenterWrapper: SentryNSNotificationCenterWrapper {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@philipphofmann philipphofmann merged commit 6c7ab81 into 8.0.0 Dec 12, 2022
@philipphofmann philipphofmann deleted the test/fix-sentry-crash-installation branch December 12, 2022 12:26
kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants