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

Fixed tests (+ Youtube shorts in channels) #813

Merged
merged 20 commits into from
Mar 26, 2022

Conversation

litetex
Copy link
Member

@litetex litetex commented Mar 14, 2022

The following tests are currently failing when I run them locally:

Problematic test Fixed? Note
MediaCCCRecentListExtractorTest.testStreamList() ✔️ Likely an external problem → opened voc/voctoweb#609
No feedback... Applying workaround: Ignore faulty items
PeertubeCommentsExtractorTest$Default.testGetComments()
and .testGetCommentsFromCommentsInfo()
✔️ The old comments/videos got deleted → used new ones
PeertubeStreamExtractorTest$WhatIsPeertube.testLikeCount() ✔️ PeerTube video somehow has less likes now
YoutubeMixPlaylistExtractorTest$Mix.getName()
YoutubeMixPlaylistExtractorTest$MixWithIndex.getName()
✔️ Video got deleted → used a new one
YoutubeMusicSearchExtractorTest$Suggestion.testSearchCorrected()
YoutubeMusicSearchExtractorTest$Suggestion.testSearchSuggestion()
YoutubeSearchExtractorTest$Suggestion.testSearchCorrected()
✔️ Looks like YT currently has some problems in their backend:
Constantly switching between "Did you mean" and "Showing results for ..." occurs:
YTSearchSuggestionsBroken
⚠️ Disabled test for now - TODO: Create followup issue
YoutubeStreamExtractorControversialTest.testSubscriberCount() ✔️ Channel lost a ton of subscriptions (a few thousands) → decremented our expected number accordingly
YoutubeChannelExtractorTest$VSauce.testRelatedItems() ✔️ TeamNewPipe/NewPipe#8034 - Workaround: Return duration = 0 when a YT shorts video is encountered/the duration could not be processed.
⚠️ TODO: Create a followup issue

@opusforlife2

This comment was marked as resolved.

@litetex

This comment was marked as resolved.

@litetex litetex added the bug Issue is related to a bug label Mar 17, 2022
@litetex litetex changed the title Fixed some tests Fixed tests Mar 17, 2022
@litetex litetex changed the title Fixed tests Fixed tests (+ Youtube shorts in channels) Mar 17, 2022
@litetex litetex marked this pull request as ready for review March 17, 2022 16:41
@litetex
Copy link
Member Author

litetex commented Mar 17, 2022

Tested the changes with NewPipe and they work fine.
Shorts videos in channels now no longer throw an error and simply contain no duration:
grafik


Followup after merge:

  • ⚠️ Remind @litetex to create the issues or create them yourself
  • Once this is merged we should update NewPipe

Copy link
Member

@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.

LGTM

@opusforlife2
Copy link
Collaborator

Shorts videos in channels now no longer throw an error and simply contain no duration

When I did this TeamNewPipe/NewPipe#8034 (comment), new shorts showed with their correct duration. Is it an intermittent change that will eventually become permanent?

@litetex
Copy link
Member Author

litetex commented Mar 17, 2022

When I did this TeamNewPipe/NewPipe#8034 (comment), new shorts showed with their correct duration. Is it an intermittent change that will eventually become permanent?

From: TeamNewPipe/NewPipe#8034 (comment)

...the change is currently rolling out and replaces the duration of shorts by a Shorts icon.

@opusforlife2
Copy link
Collaborator

Indeed. The "Not loaded" channel number is constant for me now. No change even after repeated refreshes.

Stypox
Stypox previously requested changes Mar 19, 2022
@@ -130,6 +130,7 @@ public static void setUp() throws Exception {
@Override public InfoItem.InfoType expectedInfoItemType() { return InfoItem.InfoType.CHANNEL; }
}

@Disabled("Currently constantly switching between \"Did you mean\" and \"Showing results for ...\" occurs")
Copy link
Member

Choose a reason for hiding this comment

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

You could make this @MockOnly instead for now, since we still want to test that the code correctly extracts the suggestion from the stored mocks.

Copy link
Member Author

@litetex litetex Mar 19, 2022

Choose a reason for hiding this comment

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

Useless in my opinion because the tests are based on outdated data when running the test.
The actual extraction inside e.g. NewPipe can still fail and the test says that everything is ok.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but at least the test can be used to verify that code changes inside NewPipeExtractor don't break currently working features.

Copy link
Member Author

@litetex litetex Mar 20, 2022

Choose a reason for hiding this comment

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

Note: "Fixed" inside YoutubeSearchExtractorTest.java but YoutubeMusicSearchExtractorTest$Suggestion has no mock data, so I can't fix it there...

@litetex
Copy link
Member Author

litetex commented Mar 19, 2022

Update:

@litetex litetex requested a review from Stypox March 20, 2022 17:10
@litetex litetex mentioned this pull request Mar 26, 2022
1 task
@litetex litetex dismissed Stypox’s stale review March 26, 2022 19:56

Stypox is for the next weeks really busy, the problems got fixed and this can't wait any longer

@litetex litetex removed the request for review from Stypox March 26, 2022 20:08
@litetex litetex merged commit 5a18730 into TeamNewPipe:dev Mar 26, 2022
@litetex litetex deleted the fix-test-2022-03 branch March 26, 2022 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants