feat: Enhance the UIViewController breadcrumbs with more data #1945

kevinrenskers · 2022-07-06T16:51:31Z

📜 Description

We're adding more UIViewController information in the viewDidAppear breadcrumbs.

title
beingPresented
presentingViewController
parentViewController
window info, including if this is the window's rootViewController, if the window if the key window, the window's level

💡 Motivation and Context

Closes #1727

💚 How did you test it?

By running the iOS-Swift sample app.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

github-actions · 2022-07-06T16:52:10Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 0df8b25

* master: ref: Remove unused SentryCrashDeadlock (#1941) feat: Add screenshot at crash (#1920) Add code docs for SentryScope (#1942)

* master: meta: Add Pre Commit Hook (#1946)

kevinrenskers · 2022-07-07T09:33:02Z

Question: how would we add unit tests for all these properties? Like the window, presentingViewController, beingPresented, etc. Does it make sense to create such a view hierarchy in a unit test? Can we even add a presentingViewController in the first place?

…ector

* master: build: Remove trailing-whitespace (#1953) ci: Compile iOS13-Swift sample (#1951) release: 7.20.0 meta: Fix changelog (#1950) feat: Add sample rate in the baggage header, remove Userid and Transaction (#1936) build: Upate Brewfile.lock and Gemfile.lock (#1947)

brustolin · 2022-07-08T11:13:33Z

Question: how would we add unit tests for all these properties? Like the window, presentingViewController, beingPresented, etc. Does it make sense to create such a view hierarchy in a unit test? Can we even add a presentingViewController in the first place?

I find it odd to test this with unit tests, maybe we could test this with an UITest.
Create a viewController with a UILAbel that displays the breadcrumbs, this way you can assert the label content. Or something like this.

* master: fix: Properly sanitize the event context and SDK info (#1943)

kevinrenskers · 2022-07-08T12:06:45Z

Isn't that overkill just to test beingPresented, presentingViewController, and the window stuff? Do we even have UITests in place at the moment? But yeah not sure how else to test it either :/

brustolin · 2022-07-08T12:39:29Z

Isn't that overkill just to test beingPresented, presentingViewController, and the window stuff? Do we even have UITests in place at the moment? But yeah not sure how else to test it either :/

Yes, we have UI tests. You can find them at iOS-SwiftUITests project.
Since it's a pretty easy test to create, I wouldn't consider it an overkill.
We need to test is somehow, and it looks like unit tests are not the way to go.

kevinrenskers · 2022-07-08T12:46:49Z

Ah, I missed those. Will dig in!

brustolin

LGTM. Just need to take a look at CI.
Looks like Sauce Labs have a problem, but there is other tests failing.

kevinrenskers · 2022-07-11T08:26:46Z

The iOS tests failed because of testMultipleListeners again, which should hopefully be solved if we merge master into this branch. It's unrelated to this feature.

brustolin · 2022-07-11T08:28:36Z

The iOS tests failed because of testMultipleListeners again, which should hopefully be solved if we merge master into this branch. It's unrelated to this feature.

There is also Build / Sample iOS13-Swift action failing

kevinrenskers · 2022-07-11T08:33:56Z

Good point, that's fixed now.

* master: feat: Add a new enableAutoBreadcrumbTracking option (#1958) fix: Don't send error 429 as network_error (#1957) build: Format code only on specific files (#1956) Update SentryANRTrackerTests.swift (#1955)

kevinrenskers · 2022-07-12T09:43:38Z

LGTM. Just need to take a look at CI.
Looks like Sauce Labs have a problem, but there is other tests failing.

Can someone take that look at CI? I don't know what's going on there. It's not the test itself that's failing but the whole build itself?

brustolin · 2022-07-12T09:50:23Z

LGTM. Just need to take a look at CI.
Looks like Sauce Labs have a problem, but there is other tests failing.

Can someone take that look at CI? I don't know what's going on there. It's not the test itself that's failing but the whole build itself?

It is complaining that you added 'import Combine' in ViewController.swift. Do you really need this?

kevinrenskers · 2022-07-12T09:52:15Z

Ah! Good find. Nah, at first I wanted to use Combine to observe changes in the breadcrumbs, but since the sample app runs on iOS 9 (🤯) that didn't work. Forgot to remove the import though.

kevinrenskers · 2022-07-12T10:31:48Z

Now I'm getting actual test failures in CI, while they complete just fine locally. "Home Screen doesn't exist" - completely unrelated to my changes.

brustolin · 2022-07-12T10:35:33Z

Now I'm getting actual test failures in CI, while they complete just fine locally. "Home Screen doesn't exist" - completely unrelated to my changes.

Im under the impression Sauce Labs is messing with something.
@philipphofmann Do you have any idea what's happening?

philipphofmann · 2022-07-12T14:58:24Z

Im under the impression Sauce Labs is messing with something.
@philipphofmann Do you have any idea what's happening?

After looking at the test logs from SauceLabs, I don't think that SauceLabs is the problem, but rather our tests or the new functionality breaks something.

kevinrenskers · 2022-07-12T16:19:03Z

Some test say that they can't even find the home screen anymore, which it checks by looking for captureMessageButton. And I've not done anything to the existing views 🤔

In another run the newly added test fails. I think it's because of the other that the notifications come in: sometimes the one from the navigation controller comes after the view controller, and thus the message we're checking for doesn't match. This is such a wonky way to test this functionality. But I think I may have fixed the problem by simply checking the breadcrumb before setting the label.

Still, not sure what to do about the ones that fail to even find the home screen.

* master: ref: Mark options.sdkInfo as deprecated (#1960) feat: Add extra app start span (#1952)

kevinrenskers · 2022-07-12T17:03:49Z

Hooray, tests pass!

philipphofmann

I think we could simplify the UITest a bit.

Samples/iOS-Swift/iOS-Swift/AppDelegate.swift

philipphofmann · 2022-07-13T08:32:46Z

Samples/iOS-Swift/iOS-SwiftUITests/LaunchUITests.swift

@@ -20,6 +20,12 @@ class LaunchUITests: XCTestCase {
        super.tearDown()
    }

+    func testBreadcrumbData() {


Thanks for adding a UITest.

philipphofmann

Thanks 💯 @kevinrenskers.

philipphofmann

Ups, should be LGTM.

brustolin · 2022-07-13T08:51:05Z

Just need to fix this:

/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:61:46: error: Force Cast Violation: Force casts should be avoided. (force_cast)
/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:63:43: error: Force Cast Violation: Force casts should be avoided. (force_cast)
/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:62:41: error: Force Unwrapping Violation: Force unwrapping should be avoided. (force_unwrapping)

kevinrenskers · 2022-07-13T08:52:12Z

Just need to fix this:

/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:61:46: error: Force Cast Violation: Force casts should be avoided. (force_cast)
/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:63:43: error: Force Cast Violation: Force casts should be avoided. (force_cast)
/sentry-cocoa/Samples/iOS-Swift/iOS-Swift/ViewController.swift:62:41: error: Force Unwrapping Violation: Force unwrapping should be avoided. (force_unwrapping)

Too bad we're not getting these warnings within Xcode itself by integrating SwiftLint there as a build step :(

* master: ci: Don't run benchmarks on release (#1971) Don't track OOMs for simulators (#1970) feat: Automatic nest new spans with the ui life cycle function (#1959) docs: update some docs/comments to read a little better (#1966) ci: benchmarking updates (#1926) feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910) feat: Enhance the UIViewController breadcrumbs with more data (#1945) # Conflicts: # Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_System.h

feat: Enhance the UIViewController breadcrumbs with more data

d4639f3

getsentry-bot and others added 7 commits July 6, 2022 16:52

Format code

24ab320

Add changelog

87a13de

Merge branch 'master' into feat/extra-controller-breadcrumb-data

6ed0aac

* master: ref: Remove unused SentryCrashDeadlock (#1941) feat: Add screenshot at crash (#1920) Add code docs for SentryScope (#1942)

Add a bunch more information

8e4e078

Merge branch 'master' into feat/extra-controller-breadcrumb-data

0424de7

* master: meta: Add Pre Commit Hook (#1946)

Format code

480fe3f

Add a few tests

17a3727

kevinrenskers and others added 6 commits July 7, 2022 11:36

Make it build on macOS

f11ddda

Remove the storyboard info, not very useful

c2f2948

Added more information and solved an iOS crash about unrecognised sel…

ce09f8c

…ector

Format code

d4c35da

Add some more tests

cc4c5b0

kevinrenskers marked this pull request as ready for review July 8, 2022 10:33

kevinrenskers requested review from philipphofmann and brustolin as code owners July 8, 2022 10:33

kevinrenskers added 2 commits July 8, 2022 13:42

Merge branch 'master' into feat/extra-controller-breadcrumb-data

d62cb92

* master: fix: Properly sanitize the event context and SDK info (#1943)

Add test for parentViewController

773e10b

Added UI test: testBreadcrumbData

d00e5b4

brustolin reviewed Jul 11, 2022

View reviewed changes

Fix build error in iOS13-Swift

79682e2

Merge branch 'master' into feat/extra-controller-breadcrumb-data

341eb5a

* master: feat: Add a new enableAutoBreadcrumbTracking option (#1958) fix: Don't send error 429 as network_error (#1957) build: Format code only on specific files (#1956) Update SentryANRTrackerTests.swift (#1955)

Remove unused import

8d990e2

Fix the test

476e262

Merge branch 'master' into feat/extra-controller-breadcrumb-data

beee960

* master: ref: Mark options.sdkInfo as deprecated (#1960) feat: Add extra app start span (#1952)

philipphofmann reviewed Jul 13, 2022

View reviewed changes

Different way of getting the breadcrumb data on screen

aec148d

philipphofmann reviewed Jul 13, 2022

View reviewed changes

philipphofmann approved these changes Jul 13, 2022

View reviewed changes

Remove force cast and force unwrap

0df8b25

kevinrenskers merged commit e65226c into master Jul 13, 2022

kevinrenskers deleted the feat/extra-controller-breadcrumb-data branch July 13, 2022 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enhance the UIViewController breadcrumbs with more data #1945

feat: Enhance the UIViewController breadcrumbs with more data #1945

kevinrenskers commented Jul 6, 2022 •

edited

github-actions bot commented Jul 6, 2022 •

edited

kevinrenskers commented Jul 7, 2022 •

edited

brustolin commented Jul 8, 2022

kevinrenskers commented Jul 8, 2022

brustolin commented Jul 8, 2022

kevinrenskers commented Jul 8, 2022

brustolin left a comment

kevinrenskers commented Jul 11, 2022

brustolin commented Jul 11, 2022

kevinrenskers commented Jul 11, 2022

kevinrenskers commented Jul 12, 2022

brustolin commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022 •

edited

brustolin commented Jul 12, 2022

philipphofmann commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

philipphofmann left a comment

philipphofmann Jul 13, 2022

philipphofmann left a comment

philipphofmann left a comment

brustolin commented Jul 13, 2022

kevinrenskers commented Jul 13, 2022

feat: Enhance the UIViewController breadcrumbs with more data #1945

feat: Enhance the UIViewController breadcrumbs with more data #1945

Conversation

kevinrenskers commented Jul 6, 2022 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jul 6, 2022 • edited

kevinrenskers commented Jul 7, 2022 • edited

brustolin commented Jul 8, 2022

kevinrenskers commented Jul 8, 2022

brustolin commented Jul 8, 2022

kevinrenskers commented Jul 8, 2022

brustolin left a comment

Choose a reason for hiding this comment

kevinrenskers commented Jul 11, 2022

brustolin commented Jul 11, 2022

kevinrenskers commented Jul 11, 2022

kevinrenskers commented Jul 12, 2022

brustolin commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022 • edited

brustolin commented Jul 12, 2022

philipphofmann commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

kevinrenskers commented Jul 12, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Jul 13, 2022

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

brustolin commented Jul 13, 2022

kevinrenskers commented Jul 13, 2022

kevinrenskers commented Jul 6, 2022 •

edited

github-actions bot commented Jul 6, 2022 •

edited

kevinrenskers commented Jul 7, 2022 •

edited

kevinrenskers commented Jul 12, 2022 •

edited