-
-
Notifications
You must be signed in to change notification settings - Fork 485
Fix all tests and make everything work offline/with mocks #1332
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
Conversation
Thank you. I was really needed to get more reliable tests and better detect flaws in PRs.
Isn't it just called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As already noted, these are relly good changes. I did not review SoundcloudChartsExtractor
, but everything else related to the test changes which should be done by somebody else with more recent knowledge about the SoundCloud structure.
...r/src/test/java/org/schabi/newpipe/extractor/services/DefaultSimpleUntypedExtractorTest.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeChannelExtractorTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeCommentsExtractorTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeMixPlaylistExtractorTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubePlaylistExtractorTest.java
Show resolved
Hide resolved
...actor/src/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeSignaturesTest.java
Show resolved
Hide resolved
.../java/org/schabi/newpipe/extractor/services/youtube/YoutubeStreamLinkHandlerFactoryTest.java
Show resolved
Hide resolved
Oh I didn't notice that. Update: Nope, that's not the Top 50 anymore they seems to have completely reworked how charts work. There is also no more https://soundcloud.com/charts/top or https://soundcloud.com/charts/new just https://soundcloud.com/charts which then has links to various sets (aka playlists). https://api-v2.soundcloud.com/charts?genre=soundcloud:genres:all-music&client_id=MoLbAg35TuqjYwWVtNIKyRPFScQGMOBY&kind=top (used for |
Thanks for taking a closer look and sorry for causing confusion :) |
No problem :)
Rebased, should be gone now ^^ |
8f2c322
to
1be3bc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The commits are a bit messy but I guess there was no way to organize them better, given the huge amount of changes. I reviewed everything now and it seems ok. If any of my comments do not apply to the latest code, please ignore them, I could only review commits instead of the whole code as the browser would start lagging 😅
extractor/src/test/java/org/schabi/newpipe/extractor/GeneralNewPipeTest.java
Outdated
Show resolved
Hide resolved
extractor/src/test/java/org/schabi/newpipe/downloader/DownloaderFactory.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/org/schabi/newpipe/extractor/services/DefaultSimpleUntypedExtractorTest.java
Show resolved
Hide resolved
...rc/test/java/org/schabi/newpipe/extractor/services/bandcamp/BandcampSearchExtractorTest.java
Outdated
Show resolved
Hide resolved
...st/java/org/schabi/newpipe/extractor/services/media_ccc/MediaCCCRecentListExtractorTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeChannelExtractorTest.java
Outdated
Show resolved
Hide resolved
...actor/src/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeSignaturesTest.java
Outdated
Show resolved
Hide resolved
I'm not quite sure what happend there but I somehow got corrupted mocks, it didn't happen in subsequent runs so I think it might have been some sort of very weird race condition or YT just didn't like me xD
Belongs to #1335 ;) |
@Stypox A few tests are currently failing due to the rebase I will have a look at them and after that (when the CI is green) we can probably merge/SQUASH if there is nothing else Update: 2197 test passed, 108 ignored - We are good to go |
"showing results for ..." doesn't seem to be returned by the backend anymore, however the code is still present in the JS frontend.
From review: * Fix format * Use ``extractorUrl`` name * YoutubeStreamLinkHandlerFactoryTest setup with no downloader Co-Authored-By: Tobi <[email protected]>
They no longer exists and the API returns nothing for them. Overall SoundCloud seems to use a new system.
We are not C / C++ devs: * https://medium.com/@muqsithirfan/java-null-comparison-fcfbcb55ac67
Also removed irrelevant methods See TeamNewPipe#1308
Co-Authored-By: Stypox <[email protected]>
Co-Authored-By: Stypox <[email protected]>
Co-Authored-By: Stypox <[email protected]>
Co-Authored-By: Stypox <[email protected]>
Co-Authored-By: Stypox <[email protected]>
Co-Authored-By: Stypox <[email protected]>
These are currently not working, but the tests said OK because it was not checked what happens if the returned parameter is empty! -> TeamNewPipe#1339
There is currently a problem with n-Parameter deobfuscation, which is not working at all. |
public void setUp() throws Exception { | ||
super.setUp(); | ||
|
||
extractor(); // Initialize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant here is to put extractor()
directly in super.setUp()
so we don't have to ever worry about the extractor not being initialized, or would that create issues? There are various other places where I saw // Init
comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant here is to put extractor() directly in super.setUp() so we don't have to ever worry about the extractor not being initialized, or would that create issues?
Yes because there is no setUp method that creates an extractor when the class derives from InitYoutubeTest
.
Also regarding "so we don't have to ever worry about the extractor not being initialized" there is a reason why this is lazily initialized:
We had a bunch of tests where there was a extractor used exactly in this way but it was never used.
When initializing the extractor during setup all tests mark it as used despite maybe never doing so.
There are various other places where I saw // Init comments
These are all places where this is used:
I now also refactored both classes to use dedicated methods which initialize the extractor.
This requires app changes / a migration when Top 50 kiosk is set as main page. Otherwise, you'll get this crash when starting the app: Stacktrace
|
Fix all tests and make everything work offline/with mocks. Remove old mocks and generate new ones with new structure. Remove SoundCloud "Top 50" kiosk.
Fix all tests and make everything work offline/with mocks. Remove old mocks and generate new ones with new structure. Remove SoundCloud "Top 50" kiosk.
Note: SoundCloud's |
Based on/Required before merging:
List of fixes:
Made all tests mockable
Fixes #1276
-Ddownloader=MOCK
and everything will workREC
alias forRECORDING
downloader (so that I have to type less)InitNewPipeTest
(orInitYoutubeTest
for YT) so that the downloader is setup correctlyDefaultSimpleExtractorTest
andDefaultSimpleUntypedExtractorTest
region
comments that are notregion
commentsYT
Never Gonna Give You Up
video having a different title (YoutubeMixPlaylistExtractorTest)YoutubeMusicSearchExtractorTest
- "showing results for ..." doesn't seem to be returned by the backend anymore, however the code is still present in the JS frontend.YoutubeMusicSearchExtractor
to not do the same work multiple timesYoutubeStreamExtractorRelatedMixTest
randomly failing:Mix Videos can also display non-mix playlists in the related items:
Regenerated all YT mocksSoundcloud
Top 50
list no longer exists (not present at https://soundcloud.com/charts) → RemovedSoundcloudChannelTabExtractorTest#Playlists
no longer exists → Replaced with another useronFetchPage
and not always whengetInitialPage
is calledAlso:
Fixes #1283