Mock mix pl tests by XiangRongLin · Pull Request #482 · TeamNewPipe/NewPipeExtractor

XiangRongLin · 2020-12-15T15:11:00Z

I carefully read the contribution guidelines and agree to them.
I have tested the API against NewPipe.
I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

My first draft to address the points abount mocking the responses in #481. Not yet merge ready, but before that i wanted to get some opinions on this type of solution.

RecordingDownloader relays request to the existing DownloaderTestImpl, but saves the request/response pair into a json file inside a folder specific to the test class. This should be done locally when the requests that are made by a test class change.
MockDownloader relies on the json files created by the RecordingDownloader. PR/Push CI jobs should then use this downloader.
To change the downloader type during development on the local machine change DownloaderFactory.DEFAULT_DOWNLOADER
To change the downloader depending on CI jobs, add a system property to gradle commands: -Ddownloader=ABCD, where ABCD is one of DownloaderType
The downloader specified through gradle takes precedence over the one specified inside DownloaderFactory. If none is giving it falls back to that one.

Some sitenotes:

YoutubeParsingHelper holds a global state about client version and client key. The first test class that gets executed with the RecordingDownloader makes a request and gets that request saved. Other test classes also need that request saved, but since the global state is then already initialized, that request won't be made. Currently I added that request as shared and add them to all MockDownloaders
~~apache-commons-io can be removed if needed. Was needed to clear the directories before RecordingDownloader fills them.~~
Gson is used over nanojson, because nanojson couldn't serialize the responseBody
I couldn't test if gradle aggregatedJavadocs -Ddownloader=xyz works because it won't work on windows due to file encoding. I only tried gradle extractor:test -Ddownloader=xyz
~~Ignore the first (move package) and last commit (generated jsons) for easier diffs~~
What is that design pattern called, where you get a specific implementation based on an input. I don't think it's factory

XiangRongLin · 2020-12-16T07:25:23Z

Running gradle extractor:test -Ddownloader=RECORDING does use the RecordingDownloader (tried with debugger) but does not create the files, which i don't understand.

XiangRongLin · 2021-01-03T19:30:25Z

Sooo, are there any opinions about this approach?

Because i don't want to put more time into it if the approach is already not good.

Stypox · 2021-01-03T22:20:58Z

I missed this 20 days ago and I don't really have much time to dedicate to NewPipe currently, I may try to look at it the day after tomorrow.

Stypox

I like this approach, it allows for expansion and is simple to migrate to. Thank you for your dedication :-D

...test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeMixPlaylistExtractorTest.java

extractor/src/test/java/org/schabi/newpipe/downloader/DownloaderFactory.java

extractor/build.gradle

extractor/src/test/java/org/schabi/newpipe/downloader/MockDownloader.java

RecordingDownloader relies on the real downloader and saves the request/response pair into a json file. MockDownloader uses json files from above and mocks responses for specific requests.

…ables. If the system property 'downloader' is set that use that specific downloader. This is used from gradle by appending `-Ddownloader=ABCD to the command. ABCD is one of DownloaderType. The other variable is the static property `DEFAULT_DOWNLOADER` in DownloaderFactory, which can be easily changed as needed inside the IDE according to development needs`. Normal workflow would be to first use the recording downloader and afterwards only use mocks, if the requests are always staying the same.

This is needed so that a request is made for each test class when running multiple at once. This way RecordingDownloader records all necessary requests. This works as long as tests are run sequentially and not in parallel.

Only read those files in MockDownloader

XiangRongLin · 2021-01-10T19:57:51Z

@Stypox i rebased on top of dev.
The new changes are seperate commits for easier reviewing. For the actual changes see commit history.
My first sitenote about the global state in YoutubeParsingHelper i just band-aid style fixed it with a reset method.

I will regenerate the mock files, when the pr is ready to be merged.

edit: easier reviewing as in you only have to check the new commits. (after jsom generation)

extractor/src/test/java/org/schabi/newpipe/downloader/RecordingDownloader.java

TobiGr · 2021-01-11T19:03:05Z

RecordingDownloader relays request to the existing DownloaderTestImpl, but saves the request/response pair into a json file inside a folder specific to the test class. This should be done locally when the requests that are made by a test class change.

MockDownloader relies on the json files created by the RecordingDownloader. PR/Push CI jobs should then use this downloader.
To change the downloader type during development on the local machine change DownloaderFactory.DEFAULT_DOWNLOADER
To change the downloader depending on CI jobs, add a system property to gradle commands: -Ddownloader=ABCD, where ABCD is one of DownloaderType

Can you put that info to a place where it can be found later? [To be honest, I don't know a good place for that]

XiangRongLin · 2021-01-12T08:43:35Z

@TobiGr I added to the classes/methods themselves. Anywhere else it would just get out of date.

Only thing thats missing is that CI jobs should use MockDownloader, but i would add that to the CI job itself.

Stypox

Almost ready, imo

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

Stypox

Thank you, this now looks good to me. Do you still want to implement something else or is this ready @XiangRongLin ? @TobiGr I'll let you merge, since you also reviewed this PR.

XiangRongLin · 2021-01-15T06:26:45Z

I'll just recreate the json files later. Then it can be merged

Stypox

Looking good to me, and the relevant tests succeed. Great :-D

Stypox requested changes Jan 9, 2021

View reviewed changes

XiangRongLin added 12 commits January 10, 2021 19:36

Move DownloaderTestImpl into downloader package

4dad3d6

Generate equals and hashCode for Request

1bcb9c7

Add additional downloader implementations

7c40fb8

RecordingDownloader relies on the real downloader and saves the request/response pair into a json file. MockDownloader uses json files from above and mocks responses for specific requests.

Adjust YoutubeMixPlaylistExtractorTest to use DownloaderFactory

285c26e

Add generated json and manually copy client_version.json

f8aa989

fix import due to downloader package move

f447a7a

Add method to allow resetting youtube client version and key

35e2997

This is needed so that a request is made for each test class when running multiple at once. This way RecordingDownloader records all necessary requests. This works as long as tests are run sequentially and not in parallel.

Add resource path base to DownloaderFactory

255c726

Remove unnecessary file object creation

f91916c

Prefix mock file from RecordingDownloader with "generated_mock_"

1ea6c6c

Only read those files in MockDownloader

Remove dependency to commons-io

e82cb1e

XiangRongLin force-pushed the mock_mix_pl_test branch from 2883e4a to e82cb1e Compare January 10, 2021 19:51

XiangRongLin marked this pull request as ready for review January 10, 2021 20:00

XiangRongLin commented Jan 10, 2021

View reviewed changes

extractor/src/test/java/org/schabi/newpipe/downloader/RecordingDownloader.java Outdated Show resolved Hide resolved

Only create directories if they do not exist

63c237d

XiangRongLin added 2 commits January 12, 2021 09:37

Add documentation to testing with mocks

3455f0f

Use @link for downloader

4af50c6

Stypox requested changes Jan 14, 2021

View reviewed changes

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

Add comment for usage of YoutubeParsingHelper.resetClientVersionAndKey

6ca7123

Stypox approved these changes Jan 14, 2021

View reviewed changes

Regenerate mock files

4f81d9d

Stypox approved these changes Jan 15, 2021

View reviewed changes

Stypox merged commit c2c4d97 into TeamNewPipe:dev Jan 15, 2021

XiangRongLin deleted the mock_mix_pl_test branch January 15, 2021 12:20

XiangRongLin mentioned this pull request Jan 15, 2021

Automated testing strategy in NewPipeExtractor #481

Closed

TobiGr mentioned this pull request Jan 18, 2021

Release 0.20.10 TeamNewPipe/NewPipe#5455

Closed

14 tasks

This was referenced Jan 18, 2021

Release 0.20.10 TeamNewPipe/NewPipe#5456

Merged

Use and generate mocks for youtube tests #518

Merged

Uh oh!

Conversation

XiangRongLin commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XiangRongLin commented Dec 16, 2020

Uh oh!

XiangRongLin commented Jan 3, 2021

Uh oh!

Stypox commented Jan 3, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

XiangRongLin commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TobiGr commented Jan 11, 2021

Uh oh!

XiangRongLin commented Jan 12, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

XiangRongLin commented Jan 15, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XiangRongLin commented Dec 15, 2020 •

edited

Loading

XiangRongLin commented Jan 10, 2021 •

edited

Loading