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

Use and generate mocks for youtube tests #518

Merged
merged 10 commits into from
Feb 15, 2021

Conversation

XiangRongLin
Copy link
Collaborator

  • I carefully read the contribution guidelines and agree to them.

  • I removed some and added some other Ignores.

    • Added Ignores were mostly file encoding problems (i think)
  • Some tests are not immediatly compatible, see each commit message

@XiangRongLin
Copy link
Collaborator Author

So any opinions about this?

I think #527 is a good example on why this is necessary. Normally a change like that can be easily merged if the tests pass. But the tests suite failed there. No one knows if it's due to the change or unrelated broken/flaky tests.

@Stypox
Copy link
Member

Stypox commented Jan 24, 2021

I like this approach, as I said in #482. The only thing that I think could be improved is that you always have to remember to put YoutubeParsingHelper.resetClientVersionAndKey(); in every YouTube test. I would implement this function somewhere instead and call it in the setup method of each test:

void initNewpipeForTest(@Nullable final String mockResourcePath) { // or maybe have the downloader as argument
    YoutubeParsingHelper.resetClientVersionAndKey();
    if (mockResourcePath == null) {
        NewPipe.init(new DownloaderFactory().getDownloader()); // or whatever the signature is
    } else {
        NewPipe.init(new DownloaderFactory().getDownloader(mockResourcePath));
    }
}

@XiangRongLin
Copy link
Collaborator Author

I personally don't see much difference, since one already has to remember to use the DownloaderFactory and not DownloaderTestImpl like one usually does. If it is absolutly needed, i can add it.

Additionally that specific resetting is only needed for Youtube and not the other services. So if someone used that initNewpipeForTest for SoundClound, they would run code completly unrelated and unneeded. If the other services have global state too (haven't checked), which would require a "reset" method, then initNewpipeForTest would just get bloated.

YoutubeParsingHelper.resetClientVersionAndKey() is a quick and dirty solution, since i didn't want to work on the global state in YoutubeParsingHelper while working on this.

@TobiGr TobiGr added the youtube service, https://www.youtube.com/ label Jan 24, 2021
@XiangRongLin
Copy link
Collaborator Author

@Stypox Any update on your opinion about YoutubeParsingHelper.resetClientVersionAndKey()

I would like to get this moving

@Stypox
Copy link
Member

Stypox commented Feb 13, 2021

Sorry, I missed your response above. I didn't think that running code for unrelated services could cause unrelated and misguiding errors, so you are right, it's better to keep things separate. Everything should be well-documented then.

@XiangRongLin XiangRongLin marked this pull request as draft February 13, 2021 19:22
@XiangRongLin
Copy link
Collaborator Author

Well this was more annoying than expected. After running with the recording a few times random tests started to fail. I guess youtube was thinking i was doing a DOS against them...

@Stypox Where do you want the information, that the reset method has to be used? And has it to be part of this PR or can i address it in a seperate one? I would prefer a seperate one.

@XiangRongLin XiangRongLin marked this pull request as ready for review February 13, 2021 19:33
@Stypox
Copy link
Member

Stypox commented Feb 13, 2021

@XiangRongLin yes, you can do it in a separate one.

@XiangRongLin XiangRongLin requested a review from Stypox February 14, 2021 10:07
Copy link
Member

@Stypox Stypox 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, I think this can be merged after 0.20.10 is done with. Thank you!

@B0pol B0pol mentioned this pull request Feb 14, 2021
3 tasks
@XiangRongLin XiangRongLin merged commit 50e5718 into TeamNewPipe:dev Feb 15, 2021
@XiangRongLin XiangRongLin deleted the generate_mocks branch February 15, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants