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

[ISSUE-78] Test orchestrator support #273

Conversation

mariuszmarzec
Copy link
Contributor

@mariuszmarzec mariuszmarzec commented Jan 7, 2022

📌 References

🎩 What is the goal?

Providing support for AndroidX Orchestrator

How is it being implemented?

Compose testing is done by one of the contributor, so we have to only ensure preserving of metadata files. According to default screenshot done by facebook it is needed to move screenshots and metadata files to another temporory dir, because in every starting test screenshot tool from facebook clears screenshots-default dir.

  • Upgrading facebook to 0.14.0 version (Now metadata from facebook are save as json file instead of xml)
  • For default screenshots testing, copying screenshots and metadata files for every finished test into screenshots-default-orchestrated dir on device
  • For compose screenshots testing, copying metadata (screenshots for compose are safe during test with orchestration, no needs to moving) for every finished test into screenshots-default-compose-orchestrated dir on device

How can it be tested?

To test it, it would be worth to run record and verification on projects with/without orchestrator.

  • Use case 1: Testing in all shot-consumer-* with orchestrator and without it

@mariuszmarzec mariuszmarzec marked this pull request as draft January 7, 2022 15:51
@mariuszmarzec
Copy link
Contributor Author

@pedrovgs I have created a PR as a draft. Unit test are broken and code style. I have to fix it, but not having time in this week. Waiting for some feedback if something could be done better.

@mariuszmarzec mariuszmarzec changed the title Feature/issue 78 test orchestrator support [ISSUE-78] Test orchestrator support Jan 7, 2022
@mariuszmarzec mariuszmarzec marked this pull request as ready for review January 11, 2022 18:29
@mariuszmarzec
Copy link
Contributor Author

mariuszmarzec commented Jan 11, 2022

@pedrovgs Implementation is finished. Last thing which should be done is testing on all shot-consumers projects with added/not added orchestrator.

@stetro
Copy link

stetro commented Jan 11, 2022

Hey, thanks for introducing this, I will test this PR tomorrow in our project which also rely on orchestrator. 👍 I'll keep you updated.

@stetro
Copy link

stetro commented Jan 12, 2022

I just tried this PR with no success, maybe I did it wrong:

  1. Cloned your repo, checkout your branch
  2. RELEASE_SIGNING_ENABLED=false and incremented the version for local testing purpose
  3. ./gradlew publishToMavenLocal and used the "incremented" version in my project
  4. ./gradlew clean executeScreenshotTests -Precord but still only one picture was recorded

@mariuszmarzec
Copy link
Contributor Author

@stetro Looks like getting library from remote repository before maven local. It seems to be a matter of priority of repositories.
I think you can moven maven local repository at first place (on the list in build.gradle)
Or change version before release here https://github.com/pedrovgs/Shot/blob/121affae7874334754c68c9fd4e09bc9b4e3c09c/gradle.properties and update in target project to be 100% sure that you are using version with my changes.

@mariuszmarzec
Copy link
Contributor Author

@pedrovgs Build failed with on one of shot-consumer project. Looks like something is wrong with this example or shot in 5.12.2, but it doesn't come from current version with my changes. We will try to test it on local machines as previous.

@stetro
Copy link

stetro commented Jan 12, 2022

@stetro Looks like getting library from remote repository before maven local. It seems to be a matter of priority of repositories. I think you can moven maven local repository at first place (on the list in build.gradle) Or change version before release here https://github.com/pedrovgs/Shot/blob/121affae7874334754c68c9fd4e09bc9b4e3c09c/gradle.properties and update in target project to be 100% sure that you are using version with my changes.

Correct that's why I increased the version in step 2. To be totally sure :)

@mariuszmarzec
Copy link
Contributor Author

@stetro Ahh, right. Both version name and version code? If yes, looks like i will have to check it again.

@stetro
Copy link

stetro commented Jan 12, 2022

@stetro Ahh, right. Both version name and version code? If yes, looks like i will have to check it again.

Correct, I incremented both

@mariuszmarzec
Copy link
Contributor Author

@stetro Ok, thanks for effort, will check when i find some time.

@mariuszmarzec
Copy link
Contributor Author

@stetro I fixed all issues i found. Could you check it? @pedrovgs I have tested on all shot-consumer-* project and it worked.

@stetro
Copy link

stetro commented Jan 14, 2022

@stetro I fixed all issues i found. Could you check it? @pedrovgs I have tested on all shot-consumer-* project and it worked.

With this I unfortunately run into We couldn't find any screenshot. 🙈

@mariuszmarzec
Copy link
Contributor Author

@stetro is your project public maybe? Are you using composer? I found that composer with orchestrator doesn't work. Could you check it on emulator API 28? I remember that there is another issue on 29+

@stetro
Copy link

stetro commented Jan 14, 2022

@stetro is your project public maybe? Are you using composer? I found that composer with orchestrator doesn't work. Could you check it on emulator API 28? I remember that there is another issue on 29+

Its not using compose. Running on API 30 (only) and also a private repo 🙈 Testing with another emulator is not possible as well - If my usecase is too complex to verify and debug this, never mind, I just thought someone testing it would be helpful :)

@mariuszmarzec
Copy link
Contributor Author

@stetro I meant composer (https://github.com/composer/composer), it is different tool. But information about not using compose is important as well. I started to think something could be wrong with facebook 0.14.0, but it needs more investigation. 🤷‍♂️

@stetro
Copy link

stetro commented Jan 18, 2022

@stetro I meant composer (https://github.com/composer/composer), it is different tool. But information about not using compose is important as well. I started to think something could be wrong with facebook 0.14.0, but it needs more investigation. man_shrugging

I hope you mean https://github.com/gojuno/composer 🙊 :D Bot no, thats also not used.

@mariuszmarzec
Copy link
Contributor Author

@pedrovgs Have checked composer case and provided instruction to readme how to deal with composer + orchestrator.
I have check test-consumer-* on API 28 with disabled and enabled orchestrator, it works. I check it in my private project also and working fine.

@stetro Yeah, copied wrong link. According your issues on API 30. Is there any suspicious logs in gradlew or in logcat? I found that could be a issue with hamcrest, because shot-android is using espresso 3.30, i had kaspresso which used 3.4.0 under the hood and it makes tests failed. Whole executeScreenshotTests tasks finished successfully but tests didn't make screenshots due to failing.

@mariuszmarzec
Copy link
Contributor Author

@stetro Done testing on API 30 moment ago and works for me. Check your dependencies. I used same as used in shot-android module:

    val runner_version = "1.3.0"
    val orchestrator_version = "1.4.1"
    val android_test_services_version = "1.4.1"
    val espresso_core = "3.3.0"

    androidTestImplementation("androidx.test:core:${Dependency.runner_version}")
    androidTestImplementation("androidx.test:runner:${Dependency.runner_version}")
    androidTestImplementation("androidx.test:rules:${Dependency.runner_version}")
    androidTestUtil("androidx.test:orchestrator:${Dependency.orchestrator_version}")
    androidTestUtil("androidx.test.services:test-services:${Dependency.android_test_services_version}")
    androidTestImplementation("androidx.test.espresso:espresso-core:${Dependency.espresso_core}")
    androidTestImplementation("androidx.test.espresso:espresso-contrib:${Dependency.espresso_core}")

Some of them could be updated in shot-android to newest versions but i don't want to make this PR bigger. Updates it is good thing for next enhancement.

@pedrovgs
Copy link
Owner

Hey guys sorry for the late response :( I've got covid and couldn't check this for a while. Sorry

@pedrovgs
Copy link
Owner

@mariuszmarzec I'm reading your comments and contributions and the only thing I can say is WOW and CONGRATS. I need more time to review the details but looks like everything is in place. Could you add another step to the CI to re-run the tests with composer enabled so we can ensure we don't break it in the future? Maybe we can use existing shot-consumer projects and change the build.gradle config based on an environment variable, what do you think?

@mariuszmarzec
Copy link
Contributor Author

mariuszmarzec commented Jan 21, 2022

@pedrovgs No problem, hope you are feel better. thanks for appreciation :)

Composer is not a case. I thought i broke something, but i was wrong, composer doesn't use orchestrator by default and it was needed only to provide configuration to composer and enable it, i have added guide how to do it in README.

But i think it will be worth to add to every shot-consumer orchestrator without enable it and i will add to CI configuration steps to rerun all builds with enabling orchestration by command param

------EDIT
There is no convenient api to disable/enable orchestrator. Maybe best option will be enabling for orchestrator for one of shot-conusmer project?

------EDIT
I have worse day today, you mentioned about setting variable, seems to be best option

@mariuszmarzec
Copy link
Contributor Author

@pedrovgs Added CI check with enabled orchestrator. Work is completely done. 🎉 If any code issues to fix, of course will fix it, waiting for comments or for merge and new version release 😄

Copy link
Owner

@pedrovgs pedrovgs left a comment

Choose a reason for hiding this comment

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

Well done @mariuszmarzec. I'm merging this and releasing a new version right now so we can get feedback from the users 😃 Thank you so much for this contribution, you rock. If you don't mind, let me know what's your twitter account so I can announce the new release and thank you on twitter as well!

@pedrovgs pedrovgs merged commit 6288ac3 into pedrovgs:master Jan 24, 2022
@mariuszmarzec
Copy link
Contributor Author

@pedrovgs Hey, happy to see new release 😄 I don't have any twitter account unfortunately, no problem if there will be no contact, i was mentioned on release notes, so it is good too 😄

@mariuszmarzec mariuszmarzec deleted the feature/ISSUE-78_test_orchestrator_support branch January 25, 2022 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants