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: use Test/TestCI configs for fastfile test runs #2257

Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Oct 4, 2022

I noticed we don't set a config for test runs managed by fastlane while investigating this test failure: https://github.com/getsentry/sentry-cocoa/actions/runs/3184297261/jobs/5192538083#step:4:907.

This defaults to Release per the scan docs. This change will set it to Test locally and TestCI on CI runs.

Also:

  • add Test/TestCI configs to the relevant sample projects that are tested via Fastfile, and define the preprocessor macros TEST/TESTCI

#skip-changelog

Resolves a block for the failing test in #2205

@armcknight
Copy link
Member Author

Oh right. We don't have Test/TestCI configs for the sample apps, only for the framework target. I'll add them for the relevant targets.

@armcknight
Copy link
Member Author

FTR, this kind of change is way easier/clearer with XcodeGen.

@github-actions
Copy link

github-actions bot commented Oct 4, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.65 ms 1255.44 ms 21.79 ms
Size 20.51 KiB 333.58 KiB 313.07 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
172c95a 1220.08 ms 1251.74 ms 31.66 ms
b6ef18d 1248.35 ms 1270.16 ms 21.82 ms
a5ca27b 1231.31 ms 1252.56 ms 21.25 ms
5025d2e 1248.52 ms 1251.72 ms 3.20 ms
b172a8b 1257.68 ms 1272.38 ms 14.70 ms
4c1fab4 1222.96 ms 1251.00 ms 28.04 ms
ee127f4 1203.49 ms 1231.28 ms 27.79 ms
864c39a 1239.45 ms 1256.76 ms 17.31 ms
4a66f00 1259.84 ms 1281.66 ms 21.82 ms
e958899 1233.02 ms 1261.86 ms 28.84 ms

App size

Revision Plain With Sentry Diff
172c95a 20.51 KiB 335.57 KiB 315.06 KiB
b6ef18d 20.51 KiB 332.90 KiB 312.40 KiB
a5ca27b 20.51 KiB 331.81 KiB 311.31 KiB
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
b172a8b 20.51 KiB 331.79 KiB 311.28 KiB
4c1fab4 20.51 KiB 331.79 KiB 311.28 KiB
ee127f4 20.51 KiB 333.10 KiB 312.59 KiB
864c39a 20.51 KiB 335.57 KiB 315.06 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
e958899 20.51 KiB 331.92 KiB 311.41 KiB

Previous results on branch: armcknight/test/use-testci-configuration-from-fastfile

Startup times

Revision Plain With Sentry Diff
dba6bf4 1197.33 ms 1240.16 ms 42.83 ms
3fc00fb 1220.76 ms 1251.57 ms 30.82 ms
f176572 1248.24 ms 1263.22 ms 14.98 ms
e3aef1f 1240.24 ms 1258.84 ms 18.60 ms
91af4af 1233.06 ms 1260.30 ms 27.24 ms

App size

Revision Plain With Sentry Diff
dba6bf4 20.51 KiB 333.58 KiB 313.07 KiB
3fc00fb 20.51 KiB 333.15 KiB 312.65 KiB
f176572 20.51 KiB 333.16 KiB 312.65 KiB
e3aef1f 20.51 KiB 333.16 KiB 312.65 KiB
91af4af 20.51 KiB 333.58 KiB 313.07 KiB

@brustolin
Copy link
Contributor

Im curious to know why is relevant to have a TestCI configuration when we are dealing with UI Tests? Why not just run it the release build. Isn't the release the most restrictive configuration? If Test pass as released we are good right?

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.

LGTM

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

@kevinrenskers
Copy link
Contributor

How exactly does this fix the issue you were looking into? It seems kind of like a strange PR 🤔

Im curious to know why is relevant to have a TestCI configuration when we are dealing with UI Tests? Why not just run it the release build. Isn't the release the most restrictive configuration? If Test pass as released we are good right?

That is my feeling as well.

@armcknight
Copy link
Member Author

armcknight commented Oct 6, 2022

I wanted to use the TESTCI macro to test something in this PR, the function that returns the device model, which is a VM in CI when running in a simulator, so I wanted to capture that explicitly for the test: https://github.com/getsentry/sentry-cocoa/pull/2205/files#diff-05e47615b65b728147547548c485d4a344c93d187f9aef6e133360eb1be483c9R115

Then it occurred to me that the UI tests aren't running with that macro defined, so some other places that rely on those may not work as expected when writing new tests. I figured it'd just be cleaner to have the same configuration name vs having a mapping between configurations of different names in the sample and framework projects, moving forward.

I considered just defining TEST=1 TESTCI=1 in the sample app's Release config but that didn't seem like as good an option. I did this in a separate PR from the one linked above to see how the test suite reacted.

Although, something Pierre and I just discussed on the linked PR means this PR may not actually be necessary. If we only want a simulator model, and not the model of the mac/vm running the simulator, then this PR isn't actually needed. No, I'd still need to do something similar for some of the tests looking at the model name returned for macOS models: VMWare7,1 vs e.g. MacBookPro15,3.

@armcknight armcknight closed this Oct 6, 2022
@armcknight armcknight reopened this Oct 6, 2022
@armcknight
Copy link
Member Author

Mmm, actually I still need it for the macOS test in that PR as well: https://github.com/getsentry/sentry-cocoa/actions/runs/3194110364/jobs/5213355436#step:9:35

@armcknight armcknight merged commit 7977992 into master Oct 6, 2022
@armcknight armcknight deleted the armcknight/test/use-testci-configuration-from-fastfile branch October 6, 2022 18:10
kevinrenskers added a commit that referenced this pull request Oct 13, 2022
* master:
  feat: Custom measurements API (#2268)
  ref: Don't only update App State in the OOM tracker (#2276)
  test: generate profiles with different levels of efficiency (#2274)
  test: disable testStartUpCrash_CallsFlush as it flakes (#2275)
  build(deps): bump github/codeql-action from 2.1.26 to 2.1.27 (#2272)
  ref: Use the new loadPreviousAppState in SentryAppStartTracker (#2261)
  test: use Test/TestCI configs for fastfile test runs (#2257)
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.

4 participants