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

ref: profile sample mocking #3133

Merged
merged 16 commits into from
Jul 11, 2023
Merged

ref: profile sample mocking #3133

merged 16 commits into from
Jul 11, 2023

Conversation

armcknight
Copy link
Member

This is the last component of a long-standing desire to mock components of the profiler. It extracts the objc++ code from SentryProfilerTests.mm into an interface that can be used from Swift tests as well. This will help write finer grained tests to validate an upcoming fix, as well as speed up the profiler tests.

#skip-changelog

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #3133 (b9c22e6) into main (596ccc1) will decrease coverage by 0.015%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3133       +/-   ##
=============================================
- Coverage   89.109%   89.094%   -0.015%     
=============================================
  Files          501       501               
  Lines        53920     53901       -19     
  Branches     19294     19297        +3     
=============================================
- Hits         48048     48023       -25     
- Misses        4902      5018      +116     
+ Partials       970       860      -110     
Impacted Files Coverage Δ
...ts/SentryTests/Transaction/SentryTracerObjCTests.m 100.000% <ø> (ø)
Sources/Sentry/SentryProfiler.mm 78.086% <100.000%> (+0.204%) ⬆️
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.063% <100.000%> (+0.030%) ⬆️
Tests/SentryProfilerTests/SentryProfilerTests.mm 98.235% <100.000%> (-0.494%) ⬇️

... and 27 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1249.16 ms 1251.47 ms 2.31 ms
Size 22.84 KiB 401.67 KiB 378.83 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
561fa74 1243.27 ms 1260.62 ms 17.35 ms
aa27ac0 1214.02 ms 1227.42 ms 13.40 ms
369222e 1232.14 ms 1258.90 ms 26.76 ms
d80d410 1231.50 ms 1240.14 ms 8.64 ms
a6f8b18 1238.54 ms 1265.56 ms 27.02 ms
dacf894 1223.96 ms 1250.41 ms 26.45 ms
189b629 1245.53 ms 1270.06 ms 24.53 ms
e84bc3f 1201.49 ms 1232.82 ms 31.33 ms
adca747 1250.82 ms 1267.57 ms 16.76 ms
32e64d1 6272.19 ms 6299.50 ms 27.31 ms

App size

Revision Plain With Sentry Diff
561fa74 20.76 KiB 427.23 KiB 406.46 KiB
aa27ac0 20.76 KiB 432.21 KiB 411.45 KiB
369222e 20.76 KiB 419.67 KiB 398.91 KiB
d80d410 20.76 KiB 425.71 KiB 404.95 KiB
a6f8b18 20.76 KiB 431.87 KiB 411.11 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB
189b629 20.76 KiB 399.69 KiB 378.93 KiB
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
adca747 20.76 KiB 401.36 KiB 380.60 KiB
32e64d1 20.76 KiB 433.18 KiB 412.42 KiB

Previous results on branch: armcknight/ref/profiler-mocks

Startup times

Revision Plain With Sentry Diff
9a0bc46 1230.22 ms 1245.10 ms 14.88 ms
bc09ac9 1251.90 ms 1256.08 ms 4.19 ms
c8f477d 1246.22 ms 1263.82 ms 17.60 ms

App size

Revision Plain With Sentry Diff
9a0bc46 22.84 KiB 401.67 KiB 378.83 KiB
bc09ac9 22.84 KiB 401.67 KiB 378.83 KiB
c8f477d 22.84 KiB 401.67 KiB 378.83 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

All good but one file that seems to be in the wrong place.

Sources/Sentry/include/SentryProfiler+Test.h Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This ref improves the testability of the profiling code 💪 . Excellent. I'm only not approving cause of Dhiogo's comment #3133 (comment).

backtrace2.absoluteTimestamp = 5;
backtrace2.addresses = addresses2;

const auto backtrace2 = mockBacktrace(12345568910, 666, "testThread", 9876543210, "testQueue",
Copy link
Member

Choose a reason for hiding this comment

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

Way cleaner 💯

* This delivers a wrapper around the C++ function to create a mock backtrace for incorporation into
* profiler state that can be called from Swift tests.
*/
@interface SentryProfilerMocksSwiftCompatible : NSObject
Copy link
Member

Choose a reason for hiding this comment

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

l: Mocks are usually test doubles that simulate the behavior of a real object with the added ability to verify interactions with that object. They are used to set expectations and assert that specific methods or properties of the mocked object are called or accessed correctly during testing. What you define here is a fake. Fakes are simplified implementations of a real object that mimics its behavior to some extent. They are typically used to provide a functional substitute for a real object that is too complex, slow, or unavailable for testing.

TLDR; if you want to follow the correct testing naming terms, we should rename all the mocks in this PR to fake, but I'm also fine with keeping it. I don't want to be nitpicking on testing terms.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry but I think this is more of a mock. I think we should be careful introducing too much terminology (and therefore cognitive overhead) like fixture, double, oracle, stub, mock, fake, dummy, spy, robot... I don't think these are all standardized terms in industry.

Since this is just creating structs to use as input to a real implementation, and not actually providing an alternate implementation of an interface, there is really no doubling going on at all, so in an OOP sense, this is neither mocking nor stubbing (nor faking).

I think the intention of the code gets across so I'm hesitant to change it. We're providing predefined input to the SUT and testing the output of the system.

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me 👍 .

Base automatically changed from armcknight/ref/extract-profiler-state-code to main July 7, 2023 18:59
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

@armcknight armcknight merged commit ee3f02e into main Jul 11, 2023
65 of 66 checks passed
@armcknight armcknight deleted the armcknight/ref/profiler-mocks branch July 11, 2023 05:05
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