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

ANDROIDX_TEST_ORCHESTRATOR support #264

Closed

Conversation

VysotskiVadim
Copy link
Contributor

@VysotskiVadim VysotskiVadim commented Sep 11, 2020

Explanation

Feature is implemented according to discussion in #173.
Of course some unexpected difficulties revealed during implementation. All of them related to appending to files, because when we run tests using orchestrator, new process is created for each test. So that each test should append data to files that left from the previous test. Standard Java tooling doesn't support this kind of scenarios.

Append to XML file

XmlSerializer can't add node to the middle of the file. For example:

<a>
    <b/>
    <b/>
    <!-- Insert here -->
</a>

To append nodes to existing XML file I use RandomAccessFile, that points to position before the root closing tag. And after appending I have to add new root closing tag manually.

Write bitmap

Despite the fact that ZIP file format designed to support appending of new file to existing archive without full rewrite, this operation isn't supported. It isn't supported for TAR format as well, but because of format structure it's much easier to create workaround for TAR format. It works similar to XML workaround. I use RandomAccessFile to seek for end of the last entry, and start append new entities after the last one. I use Apache Commons Comperes to work with TAR, I'm happy to switch to any other lib/tool if you advice me.

@xiphirx , please tell me what do you think about approach in general. I will also appreciate it if you give me some advises regarding code style and structuring.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@VysotskiVadim VysotskiVadim marked this pull request as draft September 11, 2020 18:36
@VysotskiVadim VysotskiVadim changed the title ANDROIDX_TEST_ORCHESTRATOR support [Draft] ANDROIDX_TEST_ORCHESTRATOR support Sep 11, 2020
@VysotskiVadim
Copy link
Contributor Author

@xiphirx , please check the approach in the PR. If you're happy with it, I will prettify code and open non draft pull request.

@VysotskiVadim VysotskiVadim marked this pull request as ready for review September 27, 2020 13:21
@VysotskiVadim
Copy link
Contributor Author

@xiphirx , fixed everything that I didn't like in the code. Now it's ready for review from my point of view. Please take a look when you get a chance.

@xiphirx
Copy link
Contributor

xiphirx commented Sep 29, 2020

Hey thanks for taking this on! I'm currently busy with other things and will be out for about a week. I will try to get back to you asap.

A few thoughts:

  • I'm not immediately happy about the move to zip -> tar. The problem we were initially trying to solve with the zip archive was increasing adb pull speed and reliability. The issue back then was that we were pulling individual files, based on entries in metadata.xml (because we dont want to pull old files / deleted test files), which might mean 1000s of invocations to adb pull (and it turns out that if you do this its both slow and sometimes breaks). The idea behind supplying a specific run id was so that you could just pull that directory entirely and not have to worry about pulling individual files. In this world I don't think that zipping or taring would make sense (unless a benchmark would show otherwise?)
  • Metadata is in xml currently, but this is not firm. I don't think that xml lends itself to ergonomics well at all, and so I'm happy if you want to switch to a format that is more manageable. I also think that instead of trying to append to a file, it would just be safer and easier (and probably not too slow) to just deserialize the file, append in memory and re-serialize it to disk. The other thing that pops into my head here is synchronization. What guarantees do we get from the orchestrator here? Do we need to synchronize writes / reads to the metadata file? If so, how about ditching the large blob metadata file enitrely and switching to a file per test?

@VysotskiVadim
Copy link
Contributor Author

@xiphirx , thanks for the reply. My following actions then:

  1. Compare performance adb pull for single archive vs adb pull for directory with the same content.
  2. Check orchestration doc regarding guarantees.

@VysotskiVadim
Copy link
Contributor Author

Performance measurements of adb pull tar vs directory

I created a tar bundle, by running sample app. To increase screenshots count, one test generated 1001 screenshots

for (i in 0..1000) {
  Screenshot.snapActivity(activity).setName("test$i").record()
}

screenshot_bundle.tar was 111 Mb. I extracted all content to test directory. Then I pulled archive and directory.

adb pull /sdcard/screenshots/com.facebook.testing.screenshot.example.test/screenshots-default/screenshot_bundle.tar test.tar
/sdcard/screenshots/com.facebook.testi...203.3 MB/s (111338496 bytes in 0.522s)
adb pull /sdcard/screenshots/com.facebook.testing.screenshot.example.test/screenshots-default/test test3
/sdcard/screenshots/com.facebook.testing.screenshot.example.test/screenshots-default/test/: 14148 files pulled. 21.6 MB/s (99918186 bytes in 4.412s)

To make competition fair we should also count tar bundle extracting time, for this I copied bundle extracting related python code to extract.py

time python extract.py 
Pulled 14148 files from device

real	0m1.107s
user	0m0.639s
sys	0m0.463s

Results: tar: 1.6 seconds, directory 4.4 seconds. So tar bundle 2.75 times faster in my tests.

@xiphirx , what do you think about it? does the time of adb pull for directory seem acceptable for you?

@VysotskiVadim
Copy link
Contributor Author

VysotskiVadim commented Oct 10, 2020

@xiphirx ,

What guarantees do we get from the orchestrator here?

I haven't found any doc that answers this question. But I did a quick research in source code. Basically orchestrator runs tests one by one using am instrument with -w option, and executes pm clear before each test.
According to doc -w forces am instrument to wait until the instrumentation terminates before terminating itself.
Even if am instument doesn't wait for the app processes death, pm clear will definitely kill it 🙂

So the answer is only one process is alive, so no sync of writes/read is required.

@VysotskiVadim
Copy link
Contributor Author

@xiphirx , if we want to write metadata to one file, yaml would be a great option, because it's very easy to append to an existing file. But this will require some third party library to work with this format. So not sure that single yaml file is a great option

@sikrinick
Copy link

@xiphirx
If you want to, I can prepare that in terms of hacktoberfest.
We use your library for making screenshots and this one is pretty awesome.
It allows to record in pull them in a manner we need (full-height screenshots of scrollable recyclerview with images loaded, Shot doesn't allow that).

@VysotskiVadim
Copy link
Contributor Author

Got rid of tar bundle, pulling whole directory. works fine for sample project

return;
}

private String readPreviousTestRunId() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are readPreviousTestRunId and writePreviousTestRunId required? We should be able to just read the test run id off the arguments passed into the Instrumentation instance: https://developer.android.com/reference/androidx/test/platform/app/InstrumentationRegistry#getArguments()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering this

Comment on lines 17 to 18
private static final String ROOT_TAG_OPEN = "<screenshots>";
private static final String ROOT_TAG_CLOSE = "</screenshots>";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about switching to JSON + reading in the existing file + appending in memory + re-writing the entire file?

If you're worried about that taking too long or the file growing too large, then we can just save a metadata file per test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched.
@xiphirx , I was struggling a lot with python plugin, not sure that I've changed everything that should be changed, especially tests 😢 but it woks for test example
I will appreciate your help with it, advices or commits 😉

return tileName;
}

/** Delete all screenshots associated with this album */
@Override
public void cleanup() {
if (mCurrentTestRunId.equals(mPreviousTestRunId)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup function can disappear now because we write to a unique directory each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who will remove images after test? should it be plugin?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That already doesn't really happen, it just "cleans up" because a new run will overwrite images in the same directory.

We can make the plugin clear the directory on the device after pulling, yeah.

plugin/src/py/android_screenshot_tests/pull_screenshots.py Outdated Show resolved Hide resolved
VysotskiVadim and others added 11 commits October 23, 2020 09:38
Co-authored-by: Hilal Alsibai <993832+xiphirx@users.noreply.github.com>
Co-authored-by: Hilal Alsibai <993832+xiphirx@users.noreply.github.com>
Co-authored-by: Hilal Alsibai <993832+xiphirx@users.noreply.github.com>
Co-authored-by: Hilal Alsibai <993832+xiphirx@users.noreply.github.com>
Copy link
Contributor

@xiphirx xiphirx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally approving the changes, still had a few comments. Thanks for persisting and continuing to work on this, I really appreciate it!

I will try to import this code at some point this week, likely to have it merged next week.

return;
}

private String readPreviousTestRunId() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering this


move_all_files_to_different_directory(bundle_name_local_file, dir)
# clean up
shutil.rmtree(bundle_name_local_file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where you could also issue a cleanup command to the device

@@ -503,16 +496,21 @@ def pull_all(package, dir, adb_puller):
pull_images(dir, device_dir, adb_puller=adb_puller)


def pull_filtered(package, dir, adb_puller, filter_name_regex=None):
def pull_filtered(package, dir, adb_puller, testRunId, filter_name_regex=None):
device_dir = pull_metadata(package, dir, adb_puller=adb_puller)
_validate_metadata(dir)
metadata.filter_screenshots(join(dir, 'metadata.xml'), name_regex=filter_name_regex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this needs to be metadata.json, likely should be a constant value that is reused everywhere

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated.

@VysotskiVadim
Copy link
Contributor Author

VysotskiVadim commented Nov 12, 2020

@xiphirx , please wait a bit, don't merge, I've just realized that I've tested only screenshots recording. I see that there's still some python code that uses Metadata.xml. I assume that those pieces of code are dead or I just haven't tested those scenarios yet.

@VysotskiVadim
Copy link
Contributor Author

@xiphirx , I was trying to implement proper cleaning up feature, but I realized that I can't do it without having a big picture of how python plugin works. Not sure that I have enough time to explore it. So I just leave current implementation.

I tested recordDebugAndroidTestScreenshotTest and verifyDebugAndroidTestScreenshotTest, it works fine on example project. I see there're also some python tests that will probably fail if you run it (btw I don't know how to do it), but I don't want to fix it, because some of the code that is tested there seems dead for me, I can't find its usages outside the tests. Please check maybe it can be removed.

@xiphirx , please tell me if I can do anything else in the scope of this PR. Thanks

Copy link
Contributor

@xiphirx xiphirx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small things, then I'll import and test internally + merge!

import java.io.OutputStream;
import java.io.RandomAccessFile;

class RandomAccessFileOutputStream extends OutputStream {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is now not used, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, thanks

@@ -503,16 +496,21 @@ def pull_all(package, dir, adb_puller):
pull_images(dir, device_dir, adb_puller=adb_puller)


def pull_filtered(package, dir, adb_puller, filter_name_regex=None):
def pull_filtered(package, dir, adb_puller, testRunId, filter_name_regex=None):
device_dir = pull_metadata(package, dir, adb_puller=adb_puller)
_validate_metadata(dir)
metadata.filter_screenshots(join(dir, 'metadata.xml'), name_regex=filter_name_regex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated.

if (metadataFile.exists()) {
Gson gson = new Gson();
JsonReader jsonReader = new JsonReader(new FileReader(getMetadataFile()));
mMetadata = gson.fromJson(jsonReader, new TypeToken<List<Map<String, Object>>>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isnt this type token for the type List<ScreenshotMetadata> so that it matches the type of mMetadata ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

@VysotskiVadim
Copy link
Contributor Author

VysotskiVadim commented Nov 22, 2020

@xiphirx , for some reason github doesn't let me reply for the comment #264 (comment)

I checked PullScreenshotsTask.pullScreenshots and looks like gradle never passes --filter-name-regex param to python. Do you want to fix a dead code?

@xiphirx
Copy link
Contributor

xiphirx commented Nov 30, 2020

Nope thats ok. I'll hopefully merge this in this week, thank you!

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiphirx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link

@xiphirx merged this pull request in 46d36ba.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants