Skip to content

Conversation

@stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Apr 28, 2022

📜 Description

Add some ui tests using Google's Espresso for Android.
There are currently two modules, grouped under sentry-uitest:

  • sentry-uitest-android-benchmark
    This module checks the difference between two operations and check they don't pass a specified threshold.
    It's better to keep it separate from the other tests, as it requires the application not to be debuggable.
  • sentry-uitest-android
    It contains all Android end2end tests that we are interested in. Right now it contains a mock relay server that allows us to check the envelopes that were sent to relay.
    SauceLabs integration was added, too.

Right now we just check that overheads are not over certain relative/absolute thresholds.
In the future we will add a way to compare overheads over sdk releases, but we have to discuss on how to do it.

#skip-changelog

💡 Motivation and Context

We need a benchmark to be sure that profiling overhead is under a certain threshold.
We also needed to start using some ui tests, as they can check things that Unit tests cannot do.

💚 How did you test it?

🤣

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

  • Add SauceLabs integration (ci) (in another pr)
  • Add a way to measure increases in absolute values, not only with percentages (in another pr)

stefanosiano and others added 7 commits April 20, 2022 10:51
added end2end ui tests module with base test class (early version)
added mock relay server with envelope and items assertions
app cache is deleted on test start
MockRelay defaults to success response
added comments and javadoc to Benchmark module
added a README
@stefanosiano stefanosiano marked this pull request as draft April 28, 2022 18:09
@stefanosiano stefanosiano requested a review from indragiek April 28, 2022 18:09
*sentry-android-ui-tests -> sentry-uitest
*benchmark -> sentry-uitest-android-benchmark
*end2end -> sentry-uitest-android
Updated root build gradle file to ignore uitest modules from apiValidation, DistributionPlugin and aggregateJavadocs
Added comments and javadoc on sentry-uitest-android module
@stefanosiano stefanosiano marked this pull request as ready for review April 29, 2022 14:58
@marandaneto
Copy link
Contributor

@stefanosiano drop us a line once this is ready to be reviewed.

What's about addressing the 2 items from the next steps in this PR:

  • Maybe remove some details from the Benchmark tests as they are already in the end2end (appContext, testRunner)?
  • Apply detekt on test modules?

We try to keep tests as clean as possible as well, so we run detekt and friends to all of our tests as well for every other module.

1 similar comment
@marandaneto
Copy link
Contributor

@stefanosiano drop us a line once this is ready to be reviewed.

What's about addressing the 2 items from the next steps in this PR:

  • Maybe remove some details from the Benchmark tests as they are already in the end2end (appContext, testRunner)?
  • Apply detekt on test modules?

We try to keep tests as clean as possible as well, so we run detekt and friends to all of our tests as well for every other module.

@stefanosiano
Copy link
Member Author

@marandaneto
Now the pr is ready to review.
I just need to add saucelabs, but it has nothing to do with actual code, so it's fine to review now.

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #2013 (b3ae6f6) into 6.x.x (e91f397) will increase coverage by 0.27%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##              6.x.x    #2013      +/-   ##
============================================
+ Coverage     80.79%   81.07%   +0.27%     
- Complexity     3146     3217      +71     
============================================
  Files           228      230       +2     
  Lines         11645    11821     +176     
  Branches       1565     1572       +7     
============================================
+ Hits           9409     9584     +175     
- Misses         1649     1650       +1     
  Partials        587      587              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/Hub.java 75.53% <ø> (-0.20%) ⬇️
...ntry/src/main/java/io/sentry/util/StringUtils.java 81.81% <0.00%> (-18.19%) ⬇️
...main/java/io/sentry/config/PropertiesProvider.java 90.00% <0.00%> (-10.00%) ⬇️
sentry/src/main/java/io/sentry/util/HintUtils.java 96.00% <0.00%> (-4.00%) ⬇️
sentry/src/main/java/io/sentry/IHub.java 87.50% <0.00%> (-3.81%) ⬇️
sentry/src/main/java/io/sentry/protocol/App.java 94.84% <0.00%> (-1.71%) ⬇️
...java/io/sentry/JsonReflectionObjectSerializer.java 90.76% <0.00%> (-1.02%) ⬇️
...entry/src/main/java/io/sentry/NoOpTransaction.java 25.80% <0.00%> (-0.87%) ⬇️
...ry/src/main/java/io/sentry/DirectoryProcessor.java 69.23% <0.00%> (-0.59%) ⬇️
sentry/src/main/java/io/sentry/SentryClient.java 87.46% <0.00%> (-0.41%) ⬇️
... and 50 more

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 e91f397...b3ae6f6. Read the comment docs.

@marandaneto
Copy link
Contributor

@marandaneto Now the pr is ready to review. I just need to add saucelabs, but it has nothing to do with actual code, so it's fine to review now.

Cool, I'll let this one to @romtsn , Thanks.

@romtsn
Copy link
Member

romtsn commented May 10, 2022

@stefanosiano I'm happy to review it, but could you give more context on this please? Was it aligned with someone, did you have any discussions on how this will look like? (e.g. is this just a PoC for profiling or are we going to reuse it later for other benchmarks like app startup time and so on).

It's a bit hard to review a big PR without much context.

Also, regarding the e2e tests, I believe we actually wanted to run a real/test relay in a docker container alongside the test app and have assertions on their site, so I'm not sure if the current approach with the MockWebServer would actually be valuable, since we just essentially test serialization logic (which is already tested anyway). cc @bruno-garcia

@stefanosiano
Copy link
Member Author

@marandaneto @romtsn
We will eventually evaluate Android's Macrobenchmark later, but can I merge this pr?
I already have another pr to submit for benchmarks with absolute values and saucelabs full config

@romtsn
Copy link
Member

romtsn commented May 31, 2022

@stefanosiano I've already approved, so nothing is blocking you from merging really ;)

@stefanosiano
Copy link
Member Author

@romtsn yeah, I know. Just wanted a last confirmation :)

@stefanosiano stefanosiano merged commit 712115b into 6.x.x Jun 3, 2022
@stefanosiano stefanosiano deleted the tests/android-ui-tests branch June 3, 2022 12:01
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.

7 participants