Add ability to run screenshot tests on multiple devices at once #115
Conversation
|
I will get to this PR soon, it changes a lot of things and I'm currently busy syncing things in our internal repo. |
| "Screenshot testing is wonderful"); | ||
|
|
||
| return intent; | ||
| } |
There was a problem hiding this comment.
Lets consolidate these three functions into one like:
private Intent given(DetailActivity.Type type, String text) {
Intent intent = new Intent();
intent.putExtra(DetailActivity.TYPE, type);
intent.putExtra(DetailActivity.TEXT, text);
return intent;
}
and inline the differing parameters in the tests.
| @@ -21,21 +21,24 @@ | |||
| public class MainActivityTest { | |||
|
|
|||
| @Rule | |||
| public ActivityTestRule<MainActivity> mActivityTestRule = new ActivityTestRule<>(MainActivity.class); | |||
| public ActivityTestRule<MainActivity> mActivityTestRule = new ActivityTestRule<>(MainActivity.class, true, false); | |||
There was a problem hiding this comment.
We need this change if we want to launch the activity the way we do it now. If we do as before, mainActivityTestSettingsOpen is not a deterministic test.
| play_services_text = self._play_services_text() | ||
| screen_density_text = self._screen_density_text() | ||
| screen_size_text = self._screen_size_text() | ||
|
|
There was a problem hiding this comment.
We should ensure that we have valid values here, and not None. If we receive None from any of the above functions then something is broken, so we should throw an exception here.
| try: | ||
| density = int(density) | ||
| except: | ||
| return 'UNKNOWN' |
There was a problem hiding this comment.
Instead of defaulting to UNKNOWN I would rather let this exception cause an error. _screen_density only looks for a contiguous string of numbers, so this should never fail. In case it does, we shouldn't continue, especially if None was returned.
| return 'XHDPI' | ||
| elif density in range(321, 481): | ||
| return 'XXHDPI' | ||
| else: |
There was a problem hiding this comment.
nit: this else is unnecessary
| if density: | ||
| return density.group(0) | ||
|
|
||
| def _screen_size_text(self): |
There was a problem hiding this comment.
Not sure if this needs to be a function.
| api_version = ''.join(self._api_version().splitlines()) | ||
| return 'API_' + api_version | ||
|
|
||
| def _play_services_text(self): |
There was a problem hiding this comment.
Lets move this function up near _has_play_services
|
|
||
| def _has_play_services(self): | ||
| output = common.check_output( | ||
| [common.get_adb()] + ['shell', 'pm', 'dump', 'com.google.android.gms', '|', 'grep', |
There was a problem hiding this comment.
We should check adb shell pm path com.google.android.gms instead. Less output and the same information can be gathered from the output. You will also need to catch an exception here when adb returns a non zero exit code.
| play_services_text = self._play_services_text() | ||
| screen_density_text = self._screen_density_text() | ||
| screen_size_text = self._screen_size_text() | ||
|
|
There was a problem hiding this comment.
We should include the architecture in the name.
| [common.get_adb()] + ['shell', 'getprop', 'ro.build.version.sdk']) | ||
|
|
||
| def _api_version_text(self): | ||
| api_version = ''.join(self._api_version().splitlines()) |
There was a problem hiding this comment.
api_version = int(self._api_version()) ?
|
@josedlpozo has updated the pull request. View: changes |
…-for-android into features/multiple_devices_plugin
|
Hi @xiphirx! I have already updated the code with your suggestions, please let me know if you have any more doubts! |
|
@josedlpozo has updated the pull request. View: changes |
|
@josedlpozo has updated the pull request. View: changes |
|
It seems like you need to re-record the screenshots since the device name calculator takes architecture into account now. |
|
@josedlpozo has updated the pull request. View: changes |
|
Yeap, I forgot it. I have already included them! |
xiphirx
changed the title
There was a problem hiding this comment.
@xiphirx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -445,7 +450,8 @@ def main(argv): | |||
| opt_generate_png=opts.get('--generate-png'), | |||
| record=opts.get('--record'), | |||
| verify=opts.get('--verify'), | |||
| adb_puller=SimplePuller(puller_args)) | |||
| adb_puller=SimplePuller(puller_args), | |||
| device_name_calculator=DeviceNameCalculator()) | |||
There was a problem hiding this comment.
you mentioned that you're not using a DeviceNameCalculator by default, but this seems to suggest otherwise. Am I missing something?
There was a problem hiding this comment.
where I have mentioned that I'm not using a DeviceNameCalculator by default? I look at this PR and I can't see it.
I think that you are talking about this PR #113, which is closed because you requested some changes and this one was created with the changes instead of changing that.
Did you have a look at this one?
Do you have any news?
There was a problem hiding this comment.
okay yeah, we definitely need this to default to none, otherwise it's going to break existing users directory structures (and definitely some tools internally at FB :) ). The new format can be made opt-in, perhaps in the future we can turn it on by default if enough people think it's the right move
(@xiphirx is doing most of the review, I was just scanning through this for obvious breakages)
There was a problem hiding this comment.
I was thinking about a NoOpDeviceNameCalculator which returns an empty string when name method is called. This way we don't work with None's. What do you think?
By default, NoOpDeviceNameCalculator is used but users can turn DeviceNameCalculator by setting a true multipleDevices variable in their build.gradle file.
What do you think about creating a new app example project with this multiple devices feature turned on?
There was a problem hiding this comment.
Yep, NoOpDeviceNameCalculator sounds like the right design (but: it might need some more changes because the device_name_calculator right now assumes that it's going into a subdirectory, but the default mode shouldn't go into a subdirectory)
new example app: that would be fantastic, go for it
There was a problem hiding this comment.
Hey! I've updated the PR with the changes requested. I took the freedom to write the example app in Kotlin.
About this 'device_name_calculator right now assumes that it's going into a subdirectory, but the default mode shouldn't go into a subdirectory', I think that you're talking about the change in recorder.py from os.mkdir to os.makedirs. Makedirs works also if you want to create a just one directory nor only with multiple directories.
Features/pr multiple devices
|
@josedlpozo has updated the pull request. View: changes, changes since last import |
|
Hey @josedlpozo it seems like there are some merge conflicts that need to be resolved. |
…-for-android into features/multiple_devices_plugin
|
@josedlpozo has updated the pull request. View: changes, changes since last import |
|
Hey @xiphirx conflicts are resolved. Do you have any news?? |
|
Hi @josedlpozo I'm currently trying to import your code internally, it takes a bit to do so. After, I need to test a few things and then I should be able to merge it in. |
|
|
||
| # If you keep the line number information, uncomment this to | ||
| # hide the original source file name. | ||
| #-renamesourcefileattribute SourceFile |
|
|
||
| import java.io.Serializable | ||
|
|
||
| sealed class MessageType : Serializable { |
There was a problem hiding this comment.
Lets not implement Serializable here and instead just use plain Ints
| fun addition_isCorrect() { | ||
| assertEquals(4, 2 + 2) | ||
| } | ||
| } |
| def usage(): | ||
| print("usage: ./scripts/screenshot_tests/pull_screenshots com.facebook.apk.name.tests [--generate-png]", file=sys.stderr) | ||
| print ( "usage: ./scripts/screenshot_tests/pull_screenshots com.facebook.apk.name.tests [--generate-png]", file=sys.stderr) |
| @@ -171,7 +180,7 @@ def write_view_hierarchy_overlay_nodes(hierarchy, html, parent_id): | |||
| while not to_output.empty(): | |||
| node = to_output.get() | |||
| left = node[KEY_LEFT] | |||
| top = node[KEY_TOP] | |||
| top = node[KEY_TOP] | |||
- Revert some changes requested by @xiphirx - Remove serializable and mvoe to Ints
|
@josedlpozo has updated the pull request. View: changes, changes since last import |
|
Thanks again @josedlpozo |
I want to run screenshot tests in multiple devices
Issue: #112
What is the goal?
The objective for this PR is to be able to run screenshot tests in multiple devices.
How is it being implemented?
I have added a new python class called DeviceNameCalculator which is intended to calculate a unique device identifier. This is calculated using some adb commands. This device identifier will be the directory where the screenshots will be saved.
I have thought that a reliable device identifier should be based on:
For example, a device with API level 25, Google Play Services, xxhdpi as screen density and a screen of 1920x1600 the name results as API_25_GP_XXHDPI_1920_1600. This will be the folder inside screenshots folder where the screenshots will be saved.
Screenshot filename will be the same as usual.
How can it be tested?
For testing these new changes you can use app-example-androidjunitrunner project from examples folder.
Required changes after this PR
Users should clear screenshot folder and run recordMode in order to create new folders based on the device which they are run.