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

Improve stream extractor tests & various fixes #309

Merged
merged 30 commits into from
Oct 25, 2020

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Apr 9, 2020

This PR makes all stream extractor tests consistent by having them all extend the same default base class. I took inspiration from @mauriciocolli's work in #296. Note that I didn't add tests for metadata properties, that will fit in another PR after both this pr and #306 are merged.

List of test improvements:

  • Add base classes for stream extractor tests with the same structure as in Fix search errors detection and refactor search tests #296, and extend them in each and every StreamExtractorTest. Some methods are not abstract and they provide a default value; this does not imply risks (i.e. forgetting to change a default) since if a value is different from the default one the tests will fail (not silently)
  • Thumbnail urls and uploader avatar urls are only assertSecureUrl, but not compared, since they could change I think. In MediaCCC, where they were being tested before, I left those tests there untouched.
  • Peertube has many description-only tests; I didn't create a whole test class for every one of them, but left them untouched, as a full test is not really needed imo.
  • Rename *StreamExtractorDefaultTest into *StreamExtractorTest for services when there is only one file containing stream tests (that's obviously the default one ;-) )
  • Remove assert checking whether the upload date of list items is in the past. Actually, this does not have to hold true: youtube premieres turn up in search results even though they are in the future.
  • Add assertAtLeast method to utils
  • Optimize imports

List of fixes (taken from commit messages):

  • [YouTube] Fix NullPointerException on obtaining error message when there is none: return null instead, as per javadoc
  • [YouTube] Return 0 when there is no timestamp in a video url, not -2 (that internally indicates that the regex failed to match), as per javadoc
  • [YouTube] Fix frame extraction for livestreams: another json field is used in that case. Also use saved playerResponse instead of parsing the same json once again.
  • [SoundCloud] Return empty stream list instead of null in getVideoStreams() (best-practice). Also replace every instance of return new ArrayList<>(); with return Collections.emptyList();
  • [PeerTube] Return empty audio stream list instead of null in getAudioStreams()
  • [PeerTube] Fix link handler inconsistency, that used to provide API links to the user instead of the human-readable ones. Also improve stream and channel link handler tests.
  • [PeerTube] Parse timestamp from url (previously unimplemented)
  • [MediaCCC] Fix link handler inconsistency, that used to provide API links to the user instead of the human-readable ones. Use regex instead of substrings.
  • [MediaCCC] Add tests for stream and conference link handlers.
  • [MediaCCC] Implement tag metadata extraction (unrelated to the other fixes, will probably be overwritten by Extract metadata for youtube, soundcloud & mediaccc #306)
  • [MediaCCC] Return null instead of empty items collector at getRelatedStreams(), as per javadoc
  • [MediaCCC] Remove useless overridden method getOriginalUrl() that does the same as the base one
  • [MediaCCC] Return empty video-only stream list instead of null in getVideoOnlyStreams()

Copy link
Contributor

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

Other than these small things, this looks good to me. Also, your testUploadDate() seems to be incorrect for PeerTube.

@Stypox
Copy link
Member Author

Stypox commented Apr 11, 2020

Ok, I fixed everything that was suggested plus an oversight when testing video streams, check commit messages for more info. @B0pol @wb9688

On my machine all tests succeed, but they did even two days ago when PeerTube gave issues in the tarvis-ci. The issues has to do with date timezones, I think, but if the improvement I pushed now (i.e. setting the same timezone to expected and actual dates) didn't do the trick I have no idea what to do.

@wb9688
Copy link
Contributor

wb9688 commented Apr 19, 2020

@Stypox: It's rebase time! Yes, again. Anyway, I'll try to take an actual look at it tomorrow.

@Stypox Stypox force-pushed the improve-stream-tests branch 2 times, most recently from 6dd5f01 to ab85fcb Compare April 20, 2020 07:02
@Stypox
Copy link
Member Author

Stypox commented Apr 20, 2020

@wb9688 rebased and squashed some commits (in particular, I squashed all commits related to tests into only one, while living fixes into their separate commits). I learned how to rebase interactively :-D

@B0pol B0pol added multiservice Issues related to multiple services enhancement New feature or request labels Apr 20, 2020
@Stypox Stypox force-pushed the improve-stream-tests branch from ab85fcb to 505f19c Compare May 6, 2020 19:19
@Stypox
Copy link
Member Author

Stypox commented May 6, 2020

I rebased

@Stypox
Copy link
Member Author

Stypox commented May 9, 2020

Rebased

Copy link
Contributor

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

You're missing license, etc. Other than that and these small things, it seems to be good. You also need to rebase again. Also, I got the following error for SoundcloudStreamExtractorTest$LilUziVertDoWhatIWant:

expected:<java.util.GregorianCalendar[time=1469981887000,areFieldsSet=true,areAllFieldsSet=true,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=4,HOUR_OF_DAY=16,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=0,DST_OFFSET=0]> but was:<java.util.GregorianCalendar[time=1469989087000,areFieldsSet=false,areAllFieldsSet=false,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=8,HOUR_OF_DAY=20,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=3600000,DST_OFFSET=3600000]>
Expected :java.util.GregorianCalendar[time=1469981887000,areFieldsSet=true,areAllFieldsSet=true,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=4,HOUR_OF_DAY=16,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=0,DST_OFFSET=0]
Actual   :java.util.GregorianCalendar[time=1469989087000,areFieldsSet=false,areAllFieldsSet=false,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=8,HOUR_OF_DAY=20,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=3600000,DST_OFFSET=3600000]
<Click to see difference>

java.lang.AssertionError: expected:<java.util.GregorianCalendar[time=1469981887000,areFieldsSet=true,areAllFieldsSet=true,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=4,HOUR_OF_DAY=16,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=0,DST_OFFSET=0]> but was:<java.util.GregorianCalendar[time=1469989087000,areFieldsSet=false,areAllFieldsSet=false,lenient=true,zone=sun.util.calendar.ZoneInfo[id="GMT",offset=0,dstSavings=0,useDaylight=false,transitions=0,lastRule=null],firstDayOfWeek=1,minimalDaysInFirstWeek=1,ERA=1,YEAR=2016,MONTH=6,WEEK_OF_YEAR=32,WEEK_OF_MONTH=6,DAY_OF_MONTH=31,DAY_OF_YEAR=213,DAY_OF_WEEK=1,DAY_OF_WEEK_IN_MONTH=5,AM_PM=1,HOUR=8,HOUR_OF_DAY=20,MINUTE=18,SECOND=7,MILLISECOND=0,ZONE_OFFSET=3600000,DST_OFFSET=3600000]>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.schabi.newpipe.extractor.services.DefaultStreamExtractorTest.testUploadDate(DefaultStreamExtractorTest.java:139)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
	at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
	at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:32)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:93)
	at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:118)
	at sun.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:175)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:157)
	at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:404)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
	at java.lang.Thread.run(Thread.java:748)

@Stypox Stypox force-pushed the improve-stream-tests branch 2 times, most recently from fa29b78 to d22a963 Compare May 16, 2020 18:30
@Stypox
Copy link
Member Author

Stypox commented May 16, 2020

@wb9688 fixed the failing test (for some reason SoundCloud started serving a date as GMT (edit: GMT is not the issue, the issue is Rome timezone), causing a 2h difference between the expected parsed value and the actual one) and the inconsistencies you have found. Implemented metadata tests (currently only used in PeerTube, waiting for #306) and sub-channel tests. Rebased. ;-)

@Stypox Stypox force-pushed the improve-stream-tests branch 2 times, most recently from d216b05 to 95367dd Compare May 16, 2020 19:24
@Stypox
Copy link
Member Author

Stypox commented May 16, 2020

I thought the problem lied in PeerTube and SoundCloud date parsers, but actually this problem had to do with Youtube: the upload date wasn't being parsed as a GMT date, but using the device timezone, thus leading to inconsistencies. Tests were also wrong, because they tested dates using the device timezone (I did try to prevent this, but I put the correct statement on the wrong line ;-/) Now everything seems to work, both on my end and on tarvis. It's incredible how 1085 tests succeed ;-D

@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 29, 2020
@wb9688
Copy link
Contributor

wb9688 commented Jun 14, 2020

Could you rebase?

@Stypox Stypox force-pushed the improve-stream-tests branch from 95367dd to 1ef0687 Compare June 14, 2020 08:21
@Stypox
Copy link
Member Author

Stypox commented Jun 14, 2020

Done. I'll soon also push a test for the DashMpd url that you recently fixed

@Stypox
Copy link
Member Author

Stypox commented Jun 14, 2020

Done

@Stypox Stypox force-pushed the improve-stream-tests branch from 0ffd6ec to f11fe87 Compare October 24, 2020 16:49
@Stypox
Copy link
Member Author

Stypox commented Oct 24, 2020

I rebased. There are many failing tests, but all seem to be unrelated from the changes here, since they are caused by "YouTube did not provide player config even after three attempts", PeerTube accounts/channels not providing related items, SoundCloud channels & playlists with some outdated data, youtube channels with no Videos tab, YouTube comments extractor (all not touched by this PR).

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Looks good to me!
We could make a bunch of vars and arguments final, but that can wait

@Stypox
Copy link
Member Author

Stypox commented Oct 24, 2020

@TobiGr done ;-)

@TobiGr TobiGr requested review from B0pol and removed request for wb9688 October 24, 2020 19:51
@Stypox Stypox force-pushed the improve-stream-tests branch from d007fb3 to cd32e74 Compare October 25, 2020 07:10
@Stypox Stypox force-pushed the improve-stream-tests branch from cd32e74 to 57e7994 Compare October 25, 2020 07:13
@Stypox
Copy link
Member Author

Stypox commented Oct 25, 2020

@B0pol done everything

@Stypox
Copy link
Member Author

Stypox commented Oct 25, 2020

Testing apk: app-debug.zip

@TobiGr TobiGr merged commit 14c0c37 into TeamNewPipe:dev Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request multiservice Issues related to multiple services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants