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
Update robolectric, use Runner class instead of annotations #2193
Conversation
dschuermann
commented
Feb 5, 2017
•
edited
edited
- updates robolectric to 3.2.2
- uses Runner class to configure Manifest path and SDK instead of repeating the same annotation again and again
1813510
to
8749438
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.
Great idea. My feedback is really as an opportunity to improve how we use Robolectric rather than critique of the changes.
@Override | ||
protected Config buildGlobalConfig() { | ||
return new Config.Builder() | ||
.setSdk(21) |
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.
I don't know much about Robolectric testing really. Do you think this should:
a) Generally match the targetSdkVersion?
b) Generally match the minimumVersion?
c) Just be consistent?
Currently it's 21 and we're building for 22.
I assume there's scenarios where you want to test a particular function and so you need a certain version
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.
Robolectric only has a few sdk numbers that work here, and the differences are relatively minor. I think it's generally c) here with a dash of a)
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.
I would use the targetSdkVersion
which is 22. This is also the default of robolectric if we do not configure anything.
So why even configure a sdk version here? In the past robolectric did not provide all the sdk versions, they were lagging behind Android releases. I would keep the explicit configuration to make it less depended on changes in the build.gradle.
I added a commit to set sdk to 22 in both runners.
You can explicitly override the runner config by adding an annotation again, but before each test had the annotation to use sdk 21, which violates DRY.
protected Config buildGlobalConfig() { | ||
return new Config.Builder() | ||
.setSdk(21) | ||
.setManifest(Config.NONE) |
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.
I think some tests might need a config - it's how you get resources. Again I'm too novice to know whether there's a downside to always providing one.
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.
Yup, this is the runner for k9mail-library, there is a different runner for the app which defines a Manifest path here by default. Besides, you can always override this default config via annotations again.
Awesome. Thanks 👍 |