-
-
Notifications
You must be signed in to change notification settings - Fork 337
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: base unit test class #2426
Conversation
f57730c
to
0b511a0
Compare
4c4b39b
to
dcc0527
Compare
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
909e73a | 1217.78 ms | 1229.70 ms | 11.92 ms |
dcac8ad | 1238.82 ms | 1247.80 ms | 8.98 ms |
3fdb749 | 1227.42 ms | 1248.48 ms | 21.06 ms |
f91714d | 1222.06 ms | 1247.00 ms | 24.94 ms |
d10145a | 1232.65 ms | 1257.55 ms | 24.90 ms |
c9129b6 | 1231.86 ms | 1270.11 ms | 38.25 ms |
7eee302 | 1228.73 ms | 1241.94 ms | 13.21 ms |
68094b3 | 1214.14 ms | 1255.09 ms | 40.95 ms |
e2cec76 | 1189.48 ms | 1229.84 ms | 40.36 ms |
1ce879f | 1258.12 ms | 1260.90 ms | 2.78 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
909e73a | 20.75 KiB | 383.40 KiB | 362.65 KiB |
dcac8ad | 20.75 KiB | 379.11 KiB | 358.36 KiB |
3fdb749 | 20.75 KiB | 383.81 KiB | 363.06 KiB |
f91714d | 20.75 KiB | 381.87 KiB | 361.12 KiB |
d10145a | 20.75 KiB | 379.12 KiB | 358.36 KiB |
c9129b6 | 20.75 KiB | 381.81 KiB | 361.06 KiB |
7eee302 | 20.75 KiB | 374.73 KiB | 353.97 KiB |
68094b3 | 20.75 KiB | 373.94 KiB | 353.19 KiB |
e2cec76 | 20.75 KiB | 381.81 KiB | 361.06 KiB |
1ce879f | 20.75 KiB | 381.81 KiB | 361.06 KiB |
Previous results on branch: armcknight/test/base-unit-test-class
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
73045bb | 1261.35 ms | 1280.16 ms | 18.81 ms |
8b934ad | 1224.98 ms | 1248.62 ms | 23.64 ms |
a72b4ad | 1193.32 ms | 1235.46 ms | 42.14 ms |
e2a8c76 | 1248.84 ms | 1266.79 ms | 17.95 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
73045bb | 20.75 KiB | 383.80 KiB | 363.05 KiB |
8b934ad | 20.75 KiB | 376.75 KiB | 356.00 KiB |
a72b4ad | 20.75 KiB | 383.42 KiB | 362.67 KiB |
e2a8c76 | 20.75 KiB | 383.80 KiB | 363.05 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💯
if (path == NULL) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: How is that change related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I forgot to comment on this one. I added a call to sentrycrash_deleteAllReports
to clearTestState
(which was done in some of the crash handler unit tests, so I added it here to apply to all tests) but then for test cases that had never written a crash report, this would crash with a NPE. I figured this fix would also just be a nice safeguard to have in production too.
43fb922
to
671183f
Compare
- trying to delete crash records before running initialization causes a nil crash; guard for NULL in the offending function
- there were only a handful of test cases that need a nil client, just do that explicitly for each one of those and remove the repetitious calls to `givenSDKWithHub` - fix an issue with the base test class always resetting app start measurement would invoke the callback in the test case before the invocation we're interested in
dcc0527
to
61e199e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge this soon, and make others changes in a subsequent PR to avoid merge conflicts, as this touches plenty of test classes.
@armcknight, what's stopping us from merging this? Can we do additional improvements in forthcoming PRs? |
Tests are moving too quick and the merge conflicts are too nasty. I think the better way to do this refactor is going to be to do #2425 first, probably one test class at a time, and then centralize resetting test state afterwards. On top of that, there were still a handful of tests that were failing that needed more investigation. |
While working on failing tests in #2228, I was hitting various crashes due to issues that seemed to be related to statefulness in the test suites.
This PR creates a new base unit test class that subsumes the old
clearTestState
function we had and automatically runs it on everysetUp
/tearDown
call–most tests did this, but some would only call it in one or the other.Other tests had other state management in their
setUp
/tearDown
that didn't appear inclearTestState
; those have been added so that they can apply to all tests using this.Not all tests inherit from this base class yet, only the ones that called
clearTestState
. This is meant to be a step in the direction of getting all our state under control for all tests. I think another step will be moving all of the privateFixture
classes into this base class so tests are all presented with a consistent mock system, which can be modified as needed depending on the test case. See #2425TODO
Fixed by including a separate class definition for the UI tests.
- [ ] Figure out how to avoid it being used in UI test targets. Some test classes are present in both UI test and unit test targets.#skip-changelog