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

Add instrumented tests for LocalPlaylistManager.createPlaylist #5531

Merged
merged 2 commits into from
Jul 17, 2021

Conversation

XiangRongLin
Copy link
Collaborator

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

  • Add instrumented tests for LocalPlaylistManager.createPlaylist

Fixes the following issue(s)

  • No automated tests!

Relies on the following changes

Due diligence

Notes

  • Besides testing the code, this should also serve as a "how-to" which can be referenced when asking contributors to add tests to classes like this one
  • There are 2 weird behaviours which i commented. They should be adressed somewhere seperatly and not here.
  • The manager layer and DAO layer is tested together here. Mocking the DAO with Mockito makes the tests very brittle. The example here, also does it like this. See TaskLocalDataSource which should be the equivalent layout to our LocalPlaylistManager https://github.com/android/architecture-samples
  • RxJava is unpleasant to test

@XiangRongLin XiangRongLin added the codequality Improvements to the codebase to improve the code quality label Feb 5, 2021
@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@Stypox
Copy link
Member

Stypox commented Mar 18, 2021

@XiangRongLin I like this, thank you. What's the state of the PR?

@XiangRongLin
Copy link
Collaborator Author

#5360 needs to be rebased and merged, which i can do tomorrow

@XiangRongLin XiangRongLin deleted the tests branch March 20, 2021 12:50
@XiangRongLin XiangRongLin restored the tests branch March 20, 2021 12:53
@XiangRongLin XiangRongLin reopened this Mar 20, 2021
@XiangRongLin XiangRongLin marked this pull request as ready for review June 20, 2021 07:02
@XiangRongLin
Copy link
Collaborator Author

@Stypox Well i completly forgot that i did something like this.
Now it's rebased and hopefully CI will be successfull

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you, this example is really useful :-)

@XiangRongLin
Copy link
Collaborator Author

This should probably be merged after #6560, to see the test actully pass in CI

@XiangRongLin
Copy link
Collaborator Author

Rebased and tests are passing
@Stypox you need to approve because you requested changes

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you ;-)

@Stypox Stypox merged commit eef568b into TeamNewPipe:dev Jul 17, 2021
@XiangRongLin XiangRongLin deleted the tests branch July 17, 2021 11:34
@litetex
Copy link
Member

litetex commented Jul 17, 2021

@Stypox
@XiangRongLin

I think there is something wrong with the format of the kotlin files.
Whenever I build the dev branch these changes happen:

git diff
$ git diff
diff --git a/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt b/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt
index 9883105f3..211312bc0 100644
--- a/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt
+++ b/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt
@@ -27,11 +27,11 @@ class LocalPlaylistManagerTest {
     @Before
     fun setup() {
         database = Room.inMemoryDatabaseBuilder(
-                ApplicationProvider.getApplicationContext(),
-                AppDatabase::class.java
+            ApplicationProvider.getApplicationContext(),
+            AppDatabase::class.java
         )
-                .allowMainThreadQueries()
-                .build()
+            .allowMainThreadQueries()
+            .build()

         manager = LocalPlaylistManager(database)
     }
@@ -44,8 +44,8 @@ class LocalPlaylistManagerTest {
     @Test
     fun createPlaylist() {
         val stream = StreamEntity(
-                serviceId = 1, url = "https://newpipe.net/", title = "title",
-                streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
+            serviceId = 1, url = "https://newpipe.net/", title = "title",
+            streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
         )

         val result = manager.createPlaylist("name", listOf(stream))
@@ -68,13 +68,13 @@ class LocalPlaylistManagerTest {
     @Test()
     fun createPlaylist_nonExistentStreamsAreUpserted() {
         val stream = StreamEntity(
-                serviceId = 1, url = "https://newpipe.net/", title = "title",
-                streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
+            serviceId = 1, url = "https://newpipe.net/", title = "title",
+            streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
         )
         database.streamDAO().insert(stream)
         val upserted = StreamEntity(
-                serviceId = 1, url = "https://newpipe.net/2", title = "title2",
-                streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
+            serviceId = 1, url = "https://newpipe.net/2", title = "title2",
+            streamType = StreamType.VIDEO_STREAM, duration = 1, uploader = "uploader"
         )

         val result = manager.createPlaylist("name", listOf(stream, upserted))
diff --git a/app/src/androidTest/java/org/schabi/newpipe/testUtil/TrampolineSchedulerRule.kt b/app/src/androidTest/java/org/schabi/newpipe/testUtil/TrampolineSchedulerRule.kt
index 8a12d2439..75f5c6195 100644
--- a/app/src/androidTest/java/org/schabi/newpipe/testUtil/TrampolineSchedulerRule.kt
+++ b/app/src/androidTest/java/org/schabi/newpipe/testUtil/TrampolineSchedulerRule.kt
@@ -18,20 +18,20 @@ class TrampolineSchedulerRule : TestRule {
     private val scheduler = Schedulers.trampoline()

     override fun apply(base: Statement, description: Description): Statement =
-            object : Statement() {
-                override fun evaluate() {
-                    try {
-                        RxJavaPlugins.setComputationSchedulerHandler { scheduler }
-                        RxJavaPlugins.setIoSchedulerHandler { scheduler }
-                        RxJavaPlugins.setNewThreadSchedulerHandler { scheduler }
-                        RxJavaPlugins.setSingleSchedulerHandler { scheduler }
-                        RxAndroidPlugins.setInitMainThreadSchedulerHandler { scheduler }
+        object : Statement() {
+            override fun evaluate() {
+                try {
+                    RxJavaPlugins.setComputationSchedulerHandler { scheduler }
+                    RxJavaPlugins.setIoSchedulerHandler { scheduler }
+                    RxJavaPlugins.setNewThreadSchedulerHandler { scheduler }
+                    RxJavaPlugins.setSingleSchedulerHandler { scheduler }
+                    RxAndroidPlugins.setInitMainThreadSchedulerHandler { scheduler }

-                        base.evaluate()
-                    } finally {
-                        RxJavaPlugins.reset()
-                        RxAndroidPlugins.reset()
-                    }
+                    base.evaluate()
+                } finally {
+                    RxJavaPlugins.reset()
+                    RxAndroidPlugins.reset()
                 }
             }
+        }
 }

Shouldn't the last checks have failed?

@XiangRongLin
Copy link
Collaborator Author

@litetex There is a good chance that i commented out the lines which format the kotlin files while developing. Checkstyle annoys me everytime I'm testing out stuff, which was the case here. And since checkstyle is on the same line as klint both get deactivated.

If that is the case, then can you open a PR with the formatter applied?

@litetex
Copy link
Member

litetex commented Jul 19, 2021

Checkstyle annoys me everytime I'm testing out stuff, which was the case here.

Do you mean that kind of thing where the build fails because it's no checkstyle conform?
If yes, we maybe should only warn on local builds and throw an error in CI builds. Is there a known issue for this?
Otherwise I think we should open one ;)

Maybe we should also check the kotlin formatting/linting in the CI?

If that is the case, then can you open a PR with the formatter applied?

→ see #6706

@Stypox
Copy link
Member

Stypox commented Jul 19, 2021

If yes, we maybe should only warn on local builds and throw an error in CI builds. Is there a known issue for this?

No, this is not a good idea, otherwise newbies wouldn't notice that their app conflicts with checkstyle rules.
I'd rather just check if a local file (say suppress-checkstyle.txt) is present in the ignored .idea folder instead.

@Stypox Stypox mentioned this pull request Aug 4, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants