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 VolumeScreenIndividualTest and VolumeScreenThemeTest #323

Closed
luizgrp opened this issue Jul 5, 2022 · 6 comments · Fixed by #338
Closed

Fix VolumeScreenIndividualTest and VolumeScreenThemeTest #323

luizgrp opened this issue Jul 5, 2022 · 6 comments · Fixed by #338

Comments

@luizgrp
Copy link
Member

luizgrp commented Jul 5, 2022

After merging #322, build step in main branch got green.

However, the PRs that are up to date with main branch started to fail with:

com.google.android.horologist.audio.ui.VolumeScreenIndividualTest > volumeScreenAtMaximum FAILED
    java.lang.NullPointerException at VolumeScreenIndividualTest.kt:66
com.google.android.horologist.audio.ui.VolumeScreenIndividualTest > volumeScreenAtMinimum FAILED
    java.lang.NullPointerException at VolumeScreenIndividualTest.kt:49
com.google.android.horologist.audio.ui.VolumeScreenIndividualTest > volumeScreenWithLongTest FAILED
    java.lang.NullPointerException at VolumeScreenIndividualTest.kt:83

Once again, main is green, but the PRs that are up to date with main branch started to fail with:

com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[0] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[1] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[2] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[3] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[4] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[5] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
10 tests completed, 7 failed, 3 skipped
com.google.android.horologist.audio.ui.VolumeScreenThemeTest > volumeScreenThemes[6] FAILED
    java.lang.NullPointerException at VolumeScreenThemeTest.kt:56
This was referenced Jul 5, 2022
@luizgrp luizgrp changed the title Fix VolumeScreenIndividualTest Fix VolumeScreenIndividualTest and VolumeScreenThemeTest Jul 5, 2022
@luizgrp
Copy link
Member Author

luizgrp commented Jul 5, 2022

These 2 classes were renamed in #223

We need to find out answers for:

  • why we don't see these tests failing locally?
  • why are those tests failing only for the builds for the PRs and not for the main branch?

These two things cross my mind to investigate:

  1. caching that is being used in our builds in CI
  2. this plugin to only trigger tests for modules which had files changed by the PR

@luizgrp
Copy link
Member Author

luizgrp commented Jul 7, 2022

Many tests (FigmaVolumeScreenTest, MediaPlayerScreenTest, PodcastPlayerScreenTest, MediaChipTest, ShowPlaylistChipTest, SecondaryChipTest) started failing now:

com.google.android.horologist.media.ui.FigmaVolumeScreenTest > volumePlayerScreen FAILED
    java.lang.NullPointerException at FigmaVolumeScreenTest.kt:46

...

53 tests completed, 35 failed
> Task :media-ui:testDebugUnitTest FAILED

Downloading the build artifacts I can see in the test report:

java.lang.NullPointerException: goldenImage must not be null
	at app.cash.paparazzi.SnapshotVerifier$newFrameHandler$1.handle(SnapshotVerifier.kt:53)
	at com.google.android.horologist.paparazzi.WearSnapshotHandler$newFrameHandler$1.handle(WearSnapshotHandler.kt:57)
	at app.cash.paparazzi.Paparazzi$takeSnapshots$1$2.invoke(Paparazzi.kt:302)
	at app.cash.paparazzi.Paparazzi$takeSnapshots$1$2.invoke(Paparazzi.kt:295)
	at app.cash.paparazzi.Paparazzi.withTime(Paparazzi.kt:336)
	at app.cash.paparazzi.Paparazzi.takeSnapshots(Paparazzi.kt:295)
	at app.cash.paparazzi.Paparazzi.snapshot(Paparazzi.kt:211)
	at app.cash.paparazzi.Paparazzi.snapshot(Paparazzi.kt:204)
	at app.cash.paparazzi.Paparazzi.snapshot$default(Paparazzi.kt:195)
	at com.google.android.horologist.media.ui.FigmaVolumeScreenTest.volumePlayerScreen(FigmaVolumeScreenTest.kt:46)

So it looks like it's something related to our custom snapshot handler.

@luizgrp
Copy link
Member Author

luizgrp commented Jul 7, 2022

Possible scenario for goldenImage to be null:

  • SnapshotVerifier.kt:53 calls ImageIO.read(File) - code
  • ImageIO.read(File) calls ImageIO.read(ImageInputStream) - code
  • ImageIO.read(ImageInputStream) calls ImageIO.getImageReaders - code
  • ImageIO.getImageReaders catches IllegalArgumentException and returns empty - code
  • ImageIO.read(ImageInputStream) returns null - code
  • ImageIO.read(File) returns null - code

@luizgrp
Copy link
Member Author

luizgrp commented Jul 7, 2022

After merging #337 (red in CI) into main branch (green in CI), main became red in CI with the same errors.

So going back to the points listed in #323 (comment):

  1. Caching

Cache keys for the commit before and after are the same, however, the commit before could not find the cache:

Cache not found for input keys: gradle-317e5f085fa5f82709737fd17a8c56030dd712840d6d541df61930b48f0ea8f0

Whereas the commit after found it:

Cache restored from key: gradle-317e5f085fa5f82709737fd17a8c56030dd712840d6d541df61930b48f0ea8f0
  1. affectedModuleDetector plugin

This #337 only changed one file: build.yml. Would that make the plugin indicate that all modules have been affected and hence now main is failing (because it is now testing these modules that were not being tested before)?

@luizgrp
Copy link
Member Author

luizgrp commented Jul 7, 2022

I can see in the archived logs for #336:

[INFO] [amd] unknownFiles: [.github/workflows/build.yml, build.gradle], changedProjects: [], buildAll: false
[INFO] [amd] checking whether I should include :audio and my answer is false
[INFO] [amd] checking whether I should include :composables and my answer is false
[INFO] [amd] checking whether I should include :compose-layout and my answer is false
[INFO] [amd] checking whether I should include :compose-tools and my answer is false
[INFO] [amd] checking whether I should include :audio-ui and my answer is false
[INFO] [amd] checking whether I should include :media-data and my answer is false
[INFO] [amd] checking whether I should include :media-sample and my answer is false
[INFO] [amd] checking whether I should include :media and my answer is false
[INFO] [amd] checking whether I should include :media-ui and my answer is false
[INFO] [amd] checking whether I should include :media3-backend and my answer is false
[INFO] [amd] checking whether I should include :paparazzi and my answer is false
[INFO] [amd] checking whether I should include :network-awareness and my answer is false
[INFO] [amd] checking whether I should include :sample and my answer is false
[INFO] [amd] checking whether I should include :tiles and my answer is false

Which seems to answer the question above that changes to build.yml would not trigger to test all modules.

@luizgrp
Copy link
Member Author

luizgrp commented Jul 7, 2022

Given the causes seems to be unrelated to VolumeScreenIndividualTest and VolumeScreenThemeTest themselves, this is been closed by #338 and a new issue was raised in #340

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 a pull request may close this issue.

1 participant