Fix all tests #882

litetex · 2022-07-30T15:50:35Z

I carefully read the contribution guidelines and agree to them.
I have tested the API against NewPipe.

This PR fixes all tests.

It also introduces an option to change the YouTube consent cookie value, to allow fixing mixes extraction in EU.

Supersedes #871, fixes #820.

Stypox

I rebased and added a commit for #877 (comment). Now all tests succeed on my PC. The code you changed also looks good; now we should handle ContentRequiredExceptions in NewPipe and allow users to grant consent.

Stypox · 2022-08-04T08:17:34Z

extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java

@@ -73,8 +66,13 @@
 import java.util.Random;
 import java.util.regex.Pattern;

-import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
+import static org.schabi.newpipe.extractor.NewPipe.getDownloader;


Why does your editor keep moving these around? (do not do anything about it, I just see a lot of these changes in your PRs)

Looks like IntelliJ uses a different code style than Android studio (despite being based on the same IDE):

~~I will reconfigure my IntelliJ so that it uses Android Studio's settings.~~
I'm not sure what settings we should use here? IntelliJ's or Android Studios?

Maybe we should also track these settings in Git so that they are consistent between developers.

For now I used Android Studios code format, since it's already used in the files.

Maybe we should also track these settings in Git so that they are consistent between developers.

Yeah, good idea. Could you do that, since you already looked into how it works? Thanks

Not in this PR but I created an issue: #892

extractor/src/main/java/org/schabi/newpipe/extractor/exceptions/ConsentRequiredException.java

AudricV · 2022-08-04T09:34:36Z

The consent is only required for some playlists (#876) and mixes (#820) in EU.

That's why I'm strongly against allowing tracking in all YouTube requests we made. We should only do so where it is needed.

Remember that one of the main goals of the NewPipe projects is to enhance privacy.

Stypox · 2022-08-04T09:53:55Z

Yes. This PR does not allow any new consent, as it is false by default.

opusforlife2 · 2022-08-06T17:55:54Z

What I see is that the consent variable has been wrapped in a setter/getter. I assume this is so that other NPE consumers can easily show a consent prompt, e.g. Piped?

litetex · 2022-08-07T12:01:08Z

What I see is that the consent variable has been wrapped in a setter/getter. I assume this is so that other NPE consumers can easily show a consent prompt, e.g. Piped?

Yes, in theory this should be possible now
The variable is static so it's technically no getter or setter
The static variable might change in the future for now it's primarily designed as a workaround so that the tests work again

opusforlife2 · 2022-08-07T15:56:40Z

The variable is static so it's technically no getter or setter

I didn't get this bit.

litetex · 2022-08-12T17:11:23Z

@Stypox I'm done with my changes you can review again if you have time.

The "Lofi Girl"-stream got interrupted by a copyright strike and had to be restarted. Because of this a new id is now used.

Also removed the unnecessary ``public``

Also removed the unnecessary ``public`` and reformatted the code a bit

* Removed the unnecessary ``public`` * Use parameterized tests * One song got removed (which caused the test failure), replaced it with another song

The renderers and queries got changed.

Added option to choose if you want to consent or not - currently this is done by a static variable in ``YoutubeParsingHelper`` - may not be the best long-term solution but for now the tests work again (in EU countries) 🥳

The relevant change was made in TeamNewPipe#877

used code from TeamNewPipe#833

Doesn't work for kurgesagt videos anymore, used music video (copyright free) instead

litetex · 2022-08-14T13:13:03Z

Update: So there have been a bunch of merge conflicts due to the recent hotfix.

These should be fixed now.

While fixing the conflicts I also fixed 2 additional problems:

YouTube Mixes no longer work for the specified (kurzgesagt) video
The suggestion/"Did you mean..." now longer works for the specified search term

extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java

AudricV · 2022-08-14T16:27:45Z

For #882 (comment), it seems you don't have to change something, as addYoutubeHeaders() is never called except by YoutubeMixPlaylistExtractor (this method should be called everywhere, don't touch anything here because in order to respect what I said, additional changes would be needed¹), so the consent cookie is only sent on mixes.

Note that the current code as it is would not fix #876, as this consent is not applied on regular playlists.

¹Here is my idea:

Change generateConsentCookie() to generateConsentCookie(boolean), where the boolean parameter would be something like useYesValue. If useYesValue is true, return YES+; otherwise return PENDING= and the three random digits;
Change addCookieHeader(@Nonnull final Map<String, List<String>> headers) to addCookieHeader(@Nonnull final Map<String, List<String>> headers, final boolean consentValue);
Use the boolean value provided to call generateConsentCookie(boolean);
Change addYoutubeHeaders(@Nonnull final Map<String, List<String>> headers) to addYoutubeHeaders(@Nonnull final Map<String, List<String>> headers, final boolean consentValue);
Call addCookieHeader(headers, consentValue) in this method;
Use addYoutubeHeaders(headers, YoutubeParsingHelper.isConsentAccepted()) in playlists and mixes (addYoutubeHeaders should be used anywhere, but that's not related to your PR at all);
Note: singleton lists should not be used on Cookie header, to allow putting a reCAPTCHA one.

litetex · 2022-08-21T16:33:26Z

For #882 (comment), it seems you don't have to change something, as addYoutubeHeaders() is never called except by YoutubeMixPlaylistExtractor (this method should be called everywhere, don't touch anything here because in order to respect what I said, additional changes would be needed1), so the consent cookie is only sent on mixes.

Note that the current code as it is would not fix #876, as this consent is not applied on regular playlists.

1Here is my idea:

Should I react to this?

If yes: Your solution doesn't make much sense to me, you can simply call addCookieHeader or not ;)
It also adds a lot of overhead for doing technically the same.

Also with foresighted to #833:
This would be an instance specific property and I think it can be handled in the settings of an instance with e.g. a toggle.

…cript function

litetex · 2022-08-21T18:18:56Z

Note: I also fixed #902 by

doing a check if the extracted JS is valid JavaScript and if not try the fallback (Regex)
Removing a } from the regex so that it matches correctly

Outdated review

Outdated

opusforlife2 · 2022-08-26T18:29:44Z

@litetex Could you update the PR description to mention all the changes this makes, such as #882 (comment)?

litetex force-pushed the fix-all-tests branch from a93f662 to c0f20c6 Compare July 30, 2022 16:06

litetex mentioned this pull request Jul 30, 2022

Fixing up tests and the bugs they exposed #871

Closed

3 tasks

Stypox mentioned this pull request Aug 4, 2022

Use Objects.requireNonNull(). #877

Merged

2 tasks

Stypox force-pushed the fix-all-tests branch from 9396dd6 to efa8090 Compare August 4, 2022 08:21

Stypox previously requested changes Aug 4, 2022

View reviewed changes

This was referenced Aug 7, 2022

Add Content-Type header to all POST requests without an empty body #863

Merged

[YouTube] Update clients version and improve extraction of API key and client version of WEB client #890

Merged

litetex mentioned this pull request Aug 12, 2022

Codestyle in Git / IDE-Configuration in code #892

Open

litetex and others added 13 commits August 14, 2022 14:48

Fixed YoutubeStreamExtractorLivestreamTest

9e8724d

The "Lofi Girl"-stream got interrupted by a copyright strike and had to be restarted. Because of this a new id is now used.

Fixed BandcampSearchExtractorTest

e7c1225

Also removed the unnecessary ``public``

Fixed PeertubePlaylistExtractorTest

504f810

Also removed the unnecessary ``public`` and reformatted the code a bit

Fixed SoundcloudStreamLinkHandlerFactoryTest

bf3ae5e

* Removed the unnecessary ``public`` * Use parameterized tests * One song got removed (which caused the test failure), replaced it with another song

Failing test works locally without any problems. Reformatted it a bit.

d6586da

Fixed YoutubeMusicSearchExtractorTest

2b6fe29

The renderers and queries got changed.

Fixed all YTMixPlaylists

ecfc370

Added option to choose if you want to consent or not - currently this is done by a static variable in ``YoutubeParsingHelper`` - may not be the best long-term solution but for now the tests work again (in EU countries) 🥳

Fix test about exception type: NPE, not IllegalArgment

17bad6c

The relevant change was made in TeamNewPipe#877

Use preexisting ContentNotAvailableException

fc27b8a

Use Android Studios code style

c1a72b8

Fix YoutubeStreamLinkHandlerFactoryTest and parameterized the tests

844de3e

used code from TeamNewPipe#833

Fixed YoutubePlaylistLinkHandlerFactoryTest

938e69a

YoutubePlaylistLinkHandlerFactoryTest: Use parameterized tests

2a8a623

litetex force-pushed the fix-all-tests branch from a6a3630 to 7f4518c Compare August 14, 2022 13:11

litetex added 2 commits August 14, 2022 15:12

Fixed YoutubeSearchExtractorTest$Suggestion

2e36ab1

Updated mock data of YoutubeSearchExtractorTest$Suggestion

da06166

Fix YoutubeMixPlaylistExtractorTest again

5bbea3a

Doesn't work for kurgesagt videos anymore, used music video (copyright free) instead

litetex force-pushed the fix-all-tests branch from 7f4518c to da24ac4 Compare August 14, 2022 13:12

AudricV previously requested changes Aug 14, 2022

View reviewed changes

extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java Outdated Show resolved Hide resolved

extractor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java Outdated Show resolved Hide resolved

litetex added 2 commits August 21, 2022 18:41

Improved consent cookie related constants and documentation

8ff7a90

Updated consent-related mock-data

641a447

litetex force-pushed the fix-all-tests branch from 5c114da to 641a447 Compare August 21, 2022 16:44

litetex added 3 commits August 21, 2022 20:15

YoutubeThrottlingDecrypter: Patch regex

ed0a07a

YoutubeThrottlingDecrypter: Check if returned string is a valid JavaS…

9e93d6b

…cript function

Improve YoutubeThrottlingDecrypterTest

0beb55a

litetex mentioned this pull request Aug 21, 2022

[YouTube] Throttling parameter decryption is broken, decrypt function is not again fully extracted #902

Closed

AudricV approved these changes Aug 21, 2022

View reviewed changes

AudricV requested a review from Stypox August 21, 2022 19:37

litetex merged commit d6577e5 into TeamNewPipe:dev Aug 23, 2022

litetex deleted the fix-all-tests branch August 23, 2022 14:19

litetex mentioned this pull request Aug 23, 2022

YT Music: Playlist/Album extraction broken #908

Closed

AudricV mentioned this pull request Aug 23, 2022

Specific playlist broken in EU #876

Open

AudricV removed the request for review from Stypox August 24, 2022 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all tests #882

Fix all tests #882

litetex commented Jul 30, 2022 •

edited by AudricV

Loading

Stypox left a comment

Stypox Aug 4, 2022 •

edited

Loading

litetex Aug 7, 2022 •

edited

Loading

litetex Aug 7, 2022

Stypox Aug 7, 2022

litetex Aug 12, 2022

AudricV commented Aug 4, 2022

Stypox commented Aug 4, 2022

opusforlife2 commented Aug 6, 2022

litetex commented Aug 7, 2022

opusforlife2 commented Aug 7, 2022

litetex commented Aug 12, 2022

litetex commented Aug 14, 2022

AudricV commented Aug 14, 2022

litetex commented Aug 21, 2022

litetex commented Aug 21, 2022

opusforlife2 commented Aug 26, 2022

Fix all tests #882

Fix all tests #882

Conversation

litetex commented Jul 30, 2022 • edited by AudricV Loading

Stypox left a comment

Choose a reason for hiding this comment

Stypox Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

litetex Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

litetex Aug 7, 2022

Choose a reason for hiding this comment

Stypox Aug 7, 2022

Choose a reason for hiding this comment

litetex Aug 12, 2022

Choose a reason for hiding this comment

AudricV commented Aug 4, 2022

Stypox commented Aug 4, 2022

opusforlife2 commented Aug 6, 2022

litetex commented Aug 7, 2022

opusforlife2 commented Aug 7, 2022

litetex commented Aug 12, 2022

litetex commented Aug 14, 2022

AudricV commented Aug 14, 2022

litetex commented Aug 21, 2022

litetex commented Aug 21, 2022

opusforlife2 commented Aug 26, 2022

litetex commented Jul 30, 2022 •

edited by AudricV

Loading

Stypox Aug 4, 2022 •

edited

Loading

litetex Aug 7, 2022 •

edited

Loading