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.

Sign up for GitHub

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

Jump to bottom

Parse additional dex files created in uncommon way #66

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Parse additional dex files created in uncommon way #66

wants to merge 2 commits into from

Conversation

crazyformat
Copy link
Contributor 

One build tool - Buck (http://buck.build) is using uncommon approach for multidex. It is packing
additional dex files not as classesX.dex but as a pack of .jar files in assets/secondary-program-dex-jars.
This diff adds support for this approach allowing parsing of these files along with common classesX.dex.

Minors:
 - no need to print non-unique test names. This should not happen in common but some apks somehow have it
 - filter out common example test name "testFoo"

Dmitry Kulikovsky added 2 commits May 26, 2021 08:36
One build tool - Buck (http://buck.build) is using uncommon approach …
0a4740c 
…for multidex. It is packing

additional dex files not as classesX.dex but as a pack of .jar files in assets/secondary-program-dex-jars.
This diff adds support for this approach allowing parsing of these files along with common classesX.dex.

Minors:
 - no need to print non-unique test names. This should not happen in common but some apks somehow have it
 - filter out common example test name "testFoo"
- "testFoo" turns out to be too generic if used with contains, cha…
c3339df 
…nging it to a more explicit `endsWith`
kkoser
kkoser reviewed Jun 4, 2021
View reviewed changes
Copy link
Contributor

@kkoser kkoser left a comment

Choose a reason for hiding this comment

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

Is the buck behavior documented anywhere so that we can reference it? Having a reference to the output format would be good to document

parser/src/main/kotlin/com/linkedin/dex/parser/DexParser.kt
it.name.endsWith(".dex")
}
.map{ zip.readBytes() }
.first()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why this is only the first - isn't the goal to supoprt multidex by putting the extra dex filess inside this jar?

parser/src/main/kotlin/com/linkedin/dex/parser/JUnit3Extensions.kt
@@ -65,7 +65,7 @@ private fun findJUnit3Tests(dexFiles: List<DexFile>, testNames: MutableSet<TestM
private fun DexFile.findJUnit3Tests(descriptors: MutableSet<String>): List<TestMethod> {
val testClasses = findClassesWithSuperClass(descriptors)
return createTestMethods(testClasses, { classDef, _ -> findMethodIds(classDef) })
.filter { it.testName.contains("#test") }
.filter { it.testName.contains("#test") and !it.testName.endsWith("#testFoo") }
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a strange behavior to bake into a library - I imagine your code may use testFoo as a fake test, but from the libraries perspective (as well ass junit afaik) it is a valid test, so I'm not ure we should be making assumptions about intention. Thoughts?

@crazyformat
Copy link
Contributor Author

Thanks for looking into this.
You've raised absolutely reasonable concerns. I'm not happy with neither of changes.
Buck behavior is not documented anywhere, that's how apks look like but I was not able to find any examples or documentation anywhere (not even in our internal documentation :( ).
"testFoo" is another bad idea that I'm not happy about..

All together, I don't think we should move on with this pull-request.
Thanks again and sorry for wasting your time.

By the end of the day I had to move away from dex-test-parser because in some cases it is still not able to find all the instrumentation tests in the APK, there is just no evidence of some test methods in any of dex files. Apparently, we have some in-flight generated test methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkoser kkoser kkoser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@crazyformat @kkoser