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

Fix android tests #217

Closed
wants to merge 2 commits into from
Closed

Conversation

kenyee
Copy link

@kenyee kenyee commented Nov 6, 2022

The current Jetflix android tests don't build properly from the last library update. Looks like CI only runs unit tests on github?

This PR fixes the compilation errors.
Also, the cast/crew tests are broken...they don't wait for the cast/crew sections to display. When adding enough delays for them, the assertPeople check seems to hang with a ComposeIdleException when going through the lazyrow.

Also, running on command line via the command "./gradlew connectedAndroidTest" hangs for me....seems like it installs the app but then doesn't run the tests.

val natasha = Person("Scarlett Johansson", "Natasha Romanoff", "", Gender.FEMALE)
val hermione = Person("Emma Watson", "Hermione Granger", "", Gender.FEMALE)
val sparrow = Person("Johnny Depp", "Jack Sparrow", "", Gender.MALE)
val tony = Person("Al Pacino", "Tony Montana", null, Gender.MALE)
Copy link
Author

Choose a reason for hiding this comment

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

if you don't set the URL to null, you'll get a ComposeIdleException because Coil never goes into idle.


data class Credits(val cast: List<Person>, val crew: List<Person>)
data class Credits(@Stable val cast: List<Person>, @Stable val crew: List<Person>)
Copy link
Author

Choose a reason for hiding this comment

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

lists in Kotlin are mutable so Compose won't know these are immutable w/ the @stable annotation. This helped the assertPeople() check get a little bit further (manages to scroll to the 2nd position, but then hangs reading the text values)

@@ -35,6 +35,7 @@ compose_paging = { module = "androidx.paging:paging-compose", version = "1.0.0-a
compose_activity = { module = "androidx.activity:activity-compose", version = "1.7.0-alpha01" }
compose_viewModel = { module = "androidx.lifecycle:lifecycle-viewmodel-compose", version = "2.6.0-alpha02" }
compose_navigation = { module = "androidx.navigation:navigation-compose", version = "2.6.0-alpha02" }
compose_navigation_testing = { module = "androidx.navigation:navigation-testing", version = "2.6.0-alpha03" }
Copy link
Author

Choose a reason for hiding this comment

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

this testing artifact is needed for the testing.navController in the latest Compose libs

@yasinkacmaz
Copy link
Owner

Hi @kenyee, first of all, thanks for your effort in the UI test part. I am completely aware that for a while UI tests are failing and I didn't fix them intentionally. This pr will save the day for now but they won't build on a future change that breaks UI tests. I've been searching/experimenting with the ways that I can run UI tests on GitHub CI or another CI, so androidTests will run on every PR, will fail automatically and androidTest module will stay in sync with the actual codebase changes.

On this branch I am still experimenting: 111cb88
I've fixed them and it is updated locally but I haven't finalized that part.
I will try to update that pr so you can see the progress there.

@kenyee
Copy link
Author

kenyee commented Nov 6, 2022

On this branch I am still experimenting: 111cb88 I've fixed them and it is updated locally but I haven't finalized that part. I will try to update that pr so you can see the progress there.

I think I fixed more things in this PR 😄

It's totally possible to set up android tests on github actions FYI.
Here's a blog w/ some examples, but you have to use JDK11 instead: https://medium.com/upday-devs/how-to-setup-github-actions-for-android-projects-a94e8e3b0539

@yasinkacmaz
Copy link
Owner

@kenyee Yes, it is updated locally but I haven't pushed it to the repo.

Thanks for the link, yes it is possible to have UI tests on GitHub Actions.
But it wasn't pretty straightforward as it sounds 😩
First, I had a cache issue on Github Actions that needed to be addressed. I fixed that, but then UI tests were not running and they were on retry loop. I fixed that, then the duration was around 15-20minutes, then I tried to reduce the action duration...

I even tried https://github.com/cashapp/paparazzi just to have a faster option .(I still prefer actual tests running on CI)
So long story short, finally I've found a way to include UI tests, I need to finalize them(haven't been able to work on this repo lately)
So my suggestion is, let's keep this pr right now and when I present a solution for the CI part, we can discuss it. WDYT?

@kenyee
Copy link
Author

kenyee commented Nov 6, 2022

Sounds good.
I still have no idea why connectedAndroidTest hangs in this repo 🤷
Does manage to install the app but never runs anything. Also does the same thing on a physical device and different APIs.
Would be interesting to see if it works in github actions...

@yasinkacmaz
Copy link
Owner

I still have no idea why connectedAndroidTest hangs in this repo 🤷

It was hanging on Github Actions as well(If I don't give timeout it can run forever).
It took me several months to find and fix that issue. 😞
Here is the commit that fixes hanging problem:
1216840

@kenyee
Copy link
Author

kenyee commented Nov 6, 2022

I still have no idea why connectedAndroidTest hangs in this repo 🤷

It was hanging on Github Actions as well(If I don't give timeout it can run forever). It took me several months to find and fix that issue. 😞 Here is the commit that fixes hanging problem: 1216840

That's a strange hanging fix...not sure what would have caused it from those changes. If I have time, I'll see if I can figure out which bit fixed it.

@yasinkacmaz
Copy link
Owner

I still have no idea why connectedAndroidTest hangs in this repo 🤷

It was hanging on Github Actions as well(If I don't give timeout it can run forever). It took me several months to find and fix that issue. 😞 Here is the commit that fixes hanging problem: 1216840

That's a strange hanging fix...not sure what would have caused it from those changes. If I have time, I'll see if I can figure out which bit fixed it.

Yes, strange fix for me as well 😄 . It is these lines(Actually only enableForTestFailures line)

enableForTestFailures = true
maxSnapshotsForTestFailures = 2

@kenyee
Copy link
Author

kenyee commented Nov 6, 2022

Yes, strange fix for me as well 😄 . It is these lines(Actually only enableForTestFailures line)

enableForTestFailures = true maxSnapshotsForTestFailures = 2

That makes a little more sense...probably a bug in the emulator support for that in AGP. Maybe hidden permissions issue writing to storage on the device from the test runner.
Might be worth reporting on b.android.com if you can make a small repro.

@kenyee
Copy link
Author

kenyee commented Nov 6, 2022

confirmed that fixed it for me too...that's a bizarre AGP bug...

@kenyee
Copy link
Author

kenyee commented Nov 8, 2022

Had a bit of time to follow up on creating b.android.com bug reports:

@yasinkacmaz
Copy link
Owner

Hello again @kenyee, I've opened a pull request that fixes UI Tests and adds UI test GitHub action that runs on every pr.
Here you can check it: #218

I have a dilemma about duration and host:

  • If I use ubuntu, the UI test check takes around 29-30 minutes to finish.
  • If I use macOS, the check takes around 16 minutes to finish.

Since macOS minutes multiplied by 10 on Github actions: https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#minute-multipliers
That means every time I open a pull request it will take 160 minutes of my monthly allowance which is 2000 minutes listed here: https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions#included-storage-and-minutes

Apart from that decision, everything looks fine from my side.

@kenyee
Copy link
Author

kenyee commented Nov 10, 2022

  • If I use ubuntu, the UI test check takes around 29-30 minutes to finish.
  • If I use macOS, the check takes around 16 minutes to finish.

I'd suggest leaving it on Linux. You would get 5x as many PRs through it per month. The 10x multiplier is too much.
You can also make the ui-tests dependent on unit-tests passing first so that'll save some time.

Interesting that the cast/crew tests work fine in your branch but not mine, even after I used a valid URL 🤔
Will rebase when yours merges...there must be another change I missed.

@yasinkacmaz
Copy link
Owner

Yes, it is reasonable to leave it on Linux. IMHO, no need to depend on Unit tests, since it takes a long time to finish any additional waiting time will affect pull requests. I am making some changes regarding performance, so I intend to keep UI tests as is if they won't fail.

Also, I will add cancel logic on actions, whenever a pull request gets a new push or force push, it will cancel already running checks and start new ones.

@yasinkacmaz
Copy link
Owner

I've merged the pr: #218
I guess we can close this one, now you can run the tests locally.

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.

None yet

3 participants