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

abandoned: feat: added max-test-shards and client-details to additional-app-test-apks #1898

Closed
wants to merge 25 commits into from

Conversation

asadsalman
Copy link
Contributor

@asadsalman asadsalman commented May 5, 2021

Fixes #

This PR adds the ability to specify max-test-shards and client-details for tests under additional-app-test-apks like so:

additional-app-test-apks:
    - test: lib1-test.apk
      max-test-shards: 3
    - test: lib2-test.apk
      max-test-shards: 1
      client-details:
        modulename: lib2
        key2: val2

These configurations really improves support for multi-module projects, usually the thing I want to change the most is number of shards used to test a module. It also lets me differentiate between modules by setting key/values under client-details.

Test Plan

How do we know the code works?
Added unit tests.
.

Checklist

  • Documented
  • Unit tested
  • Integration tests updated

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2021

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


1 out of 2 committers have signed the CLA.
@Sloox
@asad Salman
Asad Salman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

@asadsalman Hey can you sign the CLA bot to continue? If you are having issues with it or cannot sign it for legal reasons, please open an issue and we will implement this change for you.
There also will most likely be failing tests. Could you run a./gradlew clean build locally to ensure all is clear.

test_runner/src/main/kotlin/ftl/gc/GcAndroidTestMatrix.kt Outdated Show resolved Hide resolved
@asadsalman
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@asadsalman
Copy link
Contributor Author

I wrote up this draft to get early feedback on this approach. If it looks reasonable to you, I'm happy to proceed with fixing tests/linting/doc updates etc. I based my approach on this PR. Let me know if I should proceed.

@pawelpasterz
Copy link
Contributor

cc @jan-gogo

@Sloox
Copy link
Contributor

Sloox commented May 6, 2021

recheck

@Sloox Sloox self-requested a review May 7, 2021 07:50
@bootstraponline bootstraponline changed the title feat: added max-test-shards and client-details to addtional-app-tests-apks feat: added max-test-shards and client-details to additional-app-test-apks May 7, 2021
@Sloox
Copy link
Contributor

Sloox commented May 10, 2021

Hey @asadsalman , please check slack in the firebase community, i have left you a direct message.

@asadsalman
Copy link
Contributor Author

recheck

@asadsalman asadsalman marked this pull request as ready for review May 15, 2021 07:27
Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

The implementation looks good, thanks! 👍

Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

Minor comment, else it looks fine by me 👍

test_runner/src/main/kotlin/ftl/args/ArgsToString.kt Outdated Show resolved Hide resolved
@asadsalman asadsalman marked this pull request as draft May 18, 2021 19:59
@asadsalman asadsalman changed the title feat: added max-test-shards and client-details to additional-app-test-apks abandoned: feat: added max-test-shards and client-details to additional-app-test-apks May 18, 2021
@asadsalman
Copy link
Contributor Author

This PR is now implemented by #1947 since a recent refactor introduced merge conflicts and it was easier to reimplement this as a new PR instead of rebasing and including all those new commits in this PR too.

@asadsalman asadsalman closed this May 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants