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

Fladle plugin to automatically configure additional test apks #96

Closed
runningcode opened this issue Apr 24, 2020 · 22 comments
Closed

Fladle plugin to automatically configure additional test apks #96

runningcode opened this issue Apr 24, 2020 · 22 comments
Labels
enhancement New feature or request

Comments

@runningcode
Copy link
Owner

runningcode commented Apr 24, 2020

It would be nice if fladle automatically configured and ran all your project's additional test apks in a single fladle run in order to aggregate the test reporting.

Gathering thoughts here on creating a secondary fladle plugin that would be applied in the project root and detect all the modules with androidTest apks. These test apks would then be passed to additionalTestApks as arguments.

Considerations:

  • APKs without tests should not be passed to flank since it will cause flank to fail.
  • Does ordering matter when passing to additionalTestApks?
  • How should multiple application apks be ordered with respect to other modules?

If you are currently using `additionalTestApks, could you post a snippet as to how yours in your project looks below? I want to get an idea of how the community is currently using that feature so that this plugin can mimic that behavior.

@kozaxinan
Copy link
Contributor

  • Will it still support shard?
  • Will it combine test result and test coverage?
  • Would it be better to configure plugin manually to import modules which needs to test? At least for start. Then it can be iterated to and detector that finds androidTests.

@fondesa
Copy link

fondesa commented Apr 24, 2020

APKs without tests should not be passed to flank since it will cause flank to fail.

I'm going to implement this directly inside Flank soon, to avoid to read the dex files twice (both in Fladle and Flank). In this way, Fladle can pass all the APKs independently of their test count.

Some considerations:

  • Flank currently supports up to 100 additional APKs
  • Can it be an idea to support only a single com.android.application module at the start? Otherwise, I think a good strategy would be the product N (applications) x M (library modules added as implementation/api in each application).

@runningcode
Copy link
Owner Author

We discussed this in slack and we think the NxM strategy is not necessary because the app apk is not relevant for library apks. Thus one strategy for the implementation could be to submit them to flank like so:

First app apk found
First app test apk
Second app apk found
Second app test apk
3rd app apk found
3rd app test apk
all other test apks

If there are specific cases where this strategy would not work, please comment!

@digitalbuddha
Copy link
Contributor

Hi folks, this would be great for us (dropbox) only caveat is that we want to be able to pattern match exclusions. We have modules that I would not want to send (ie ones ending with generated)

@runningcode
Copy link
Owner Author

Ah, good point. @digitalbuddha . Currently flank supports test filtering using the --test-targets flag.
You can use it in flank like so https://github.com/runningcode/fladle#testtargets.
Would that be sufficient?
Can you tell me more about the use case for filtering test apks. You mentioned filtering ones the end with generated?

@digitalbuddha
Copy link
Contributor

If I understand correctly currently we need to list all test apks. We do this by collecting outputs and pattern matching to one's we don't want and output to a file. The current proposal is to auto include all apks. I'd like to propose similar to how the coverage gather works where you'd want to gather all apks that match a certain pattern.

Something like:
TestApks: */foo.apk

That would still give folks the ability to do /. *

@runningcode
Copy link
Owner Author

runningcode commented May 10, 2020

I see. What you need is pattern matching on the directory or name of the test apk? Does this filter need to be independent of the application apk list?
Also, what is the downside to uploading all the apks and filtering out unnecessary tests using --test-targets?

@digitalbuddha
Copy link
Contributor

Independent of the app apk to keep it less surprising. The goal is to not upload the apks at all rather than upload then filter. Some of our test apks are large and only expected to be run nightly. An additional bit of info is that I have more control over apk names than test names/packages (in a large code base) .

No biggie but figured I'd ask. Thanks for all the hard work, either way it will remove custom code 😊

@runningcode
Copy link
Owner Author

Yeah, this is great feedback and keep it coming.
It sounds like a reasonable feature. The first version of this tool may not support it, but we should build it in such a way that this feature can be supported.

@bootstraponline
Copy link

I think this feature will be a huge win for Robinhood. I've asked my team to prioritize collaborating to resolve this issue. I'm open to making changes in Flank to make this easier. 🙂

@runningcode
Copy link
Owner Author

Thanks! I'll take a crack at this during the weekend.
The only thing I can think of is better docs about how additionalTestApks work. Specifically how the ordering matters and how they are executed.

Also more information about your use case is helpful. If you could print your current additionalTestApks configuration here, that would help design a plugin that would create a list in the same order.

@runningcode
Copy link
Owner Author

Starting take a look at this.
Is there a way to only provide additionalTestApks and not provide the app apk and test apk?
This way the plugin could just look at all the app and test apks it finds and submit them as a list instead of trying to pick the first app/test pair it finds and using that as the debug/test apk?

@bootstraponline
Copy link

Is there a way to only provide additionalTestApks and not provide the app apk and test apk?
This way the plugin could just look at all the app and test apks it finds and submit them as a list instead of trying to pick the first app/test pair it finds and using that as the debug/test apk?

That's not currently supported but we could add it. I think that feature request makes sense.

@runningcode
Copy link
Owner Author

@bootstraponline or @pawelpasterz do you know why the following examples are invalid?

  additional-app-test-apks:
  - test: sample-android-library/build/outputs/apk/androidTest/debug/sample-android-library-debug-androidTest.apk
  - app: sample-kotlin/build/outputs/apk/debug/sample-kotlin-debug.apk
  - test: sample-kotlin/build/outputs/apk/androidTest/debug/sample-kotlin-debug-androidTest.apk

it fails with

om.fasterxml.jackson.module.kotlin.MissingKotlinParameterException: Instantiation of [simple type, class ftl.args.yml.AppTestPair] value failed for JSON property test due to missing (therefore NULL) value for creator parameter test which is a non-nullable type
 at [Source: (StringReader); line: 19, column: 3] (through reference chain: ftl.args.yml.AndroidFlankYml["flank"]->ftl.args.yml.AndroidFlankYmlParams["additional-app-test-apks"]->java.util.ArrayList[1]->ftl.args.yml.AppTestPair["test"])

This syntax is also invalid:

  additional-app-test-apks:
    test: sample-android-library/build/outputs/apk/androidTest/debug/sample-android-library-debug-androidTest.apk
  - app: sample-kotlin/build/outputs/apk/debug/sample-kotlin-debug.apk
    test: sample-kotlin/build/outputs/apk/androidTest/debug/sample-kotlin-debug-androidTest.apk

but fails with a different error:

com.fasterxml.jackson.dataformat.yaml.snakeyaml.error.MarkedYAMLException: while parsing a block mapping
 in 'reader', line 16, column 3:
      keep-file-path: false
      ^
expected <block end>, but found '-'
 in 'reader', line 19, column 3:
      - app: /Users/nelson/AndroidStud ...
      ^

 at [Source: (BufferedReader); line: 19, column: 3]

@bootstraponline
Copy link

bootstraponline commented May 15, 2020

- app: sample-kotlin/build/outputs/apk/debug/sample-kotlin-debug.apk

This fails because you have to provide a test apk, only an app apk is optional.

@bootstraponline
Copy link

 additional-app-test-apks:
    test: sample-android-library/build/outputs/apk/androidTest/debug/sample-android-library-debug-androidTest.apk
  - app: sample-kotlin/build/outputs/apk/debug/sample-kotlin-debug.apk
    test: sample-kotlin/build/outputs/apk/androidTest/debug/sample-kotlin-debug-androidTest.apk

This is not valid YAML, it should be:

  additional-app-test-apks:
   - test: ../test_app/apks/app-debug-androidTest.apk
   - app: ../test_app/apks/app-debug-androidTest.apk
     test: ../test_app/apks/app-debug-androidTest.apk

The - indicates the start of a pair.

@runningcode
Copy link
Owner Author

Thanks @bootstraponline that helps.

When you say - indicates the start of a pair. Does that mean that in the following example the sample-one-library would be paired with the sample-two-debug.apk?
Or is it the start and end of the pair meaning that the sample-one-library would be paired instead with whatever is specified as the main debug-apk?

  additional-app-test-apks:
    test: sample-one-library/build/outputs/apk/androidTest/debug/sample-one-library-debug-androidTest.apk
  - app: sample-two/build/outputs/apk/debug/sample-two-debug.apk
    test: sample-two/build/outputs/apk/androidTest/debug/sample-two-debug-androidTest.apk

@bootstraponline
Copy link

The example you shared is not valid YAML, it won't be parsed. 🙂

gcloud:
  app: app1.apk
  test: test1.apk

flank:
 additional-app-test-apks:
   - test: test2.apk
   - app: app2.apk
     test: test3.apk

In this example there are the following pairs, each one is an individual matrix on FTL.

  • app1.apk, test1.apk
  • app1.apk, test2.apk
  • app2.apk, test3.apk

runningcode added a commit that referenced this issue May 17, 2020
This adds a new plugin called `com.osacky.fulladle` which is to be applied at the root of the project.
It scans the project's submodules and adds all the apks and test apks to additionalTestApks.

References #96
runningcode added a commit that referenced this issue May 17, 2020
This adds a new plugin called `com.osacky.fulladle` which is to be applied at the root of the project.
It scans the project's submodules and adds all the apks and test apks to additionalTestApks.

References #96
@jan-goral
Copy link

jan-goral commented May 20, 2020

Is there a way to only provide additionalTestApks and not provide the app apk and test apk?
This way the plugin could just look at all the app and test apks it finds and submit them as a list instead of trying to pick the first app/test pair it finds and using that as the debug/test apk?

That's not currently supported but we could add it. I think that feature request makes sense.

Hey @runningcode @bootstraponline, I have created pull request which can resolve this problem, I hope.

@bootstraponline
Copy link

@runningcode I think this is solved?

@runningcode
Copy link
Owner Author

Not yet, I've gotten a few reports that the plugin is configuring APKs that don't exist. I need to investigate why this is happening. I think the plugin is not ready until that is done.
If you have reported success in using the plugin, please let me know! I won't mark this as done until its working.

@runningcode
Copy link
Owner Author

Going to close this. I haven't gotten many reports that it works or it doesn't. https://runningcode.github.io/fladle/multi-module-testing/
Please file bugs if it doesn't work with your use cases. Otherwise, I'm assuming it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants