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

Automated testing strategy in NewPipeExtractor #481

Closed
XiangRongLin opened this issue Dec 12, 2020 · 3 comments
Closed

Automated testing strategy in NewPipeExtractor #481

XiangRongLin opened this issue Dec 12, 2020 · 3 comments

Comments

@XiangRongLin
Copy link
Collaborator

XiangRongLin commented Dec 12, 2020

Currently all CI builds on dev branch and PR are failing due to test failures. Then figuring out whether those are the usual suspects or caused by my own changes is a hassle and makes CI worthless.

Reason for that is the nature of unit tests relying on external services. In this case it seems Peertube is denying the connection. see https://travis-ci.org/github/TeamNewPipe/NewPipeExtractor/builds/749264136#L450
Since these tests are activly harmful, i would suggest ignoring them all for now.
There are also seem to be some SoundCloud test which are simply broken.

The long term solution is making the unit tests not rely on the external service. The expected websites would need to be saved locally as html and injected into the extractor in the tests. Additional benefits are that tests won't fail because the video/playlist/channel got deleted/changed.

A new problem that is introduced with this are missing tests for API changes of external services. For that a seperate test suite can be added, which only covers this (smoke test). Those should then NOT be run on every PR/Push but maybe nightly/weekly. Those test then also can have big buffers between executions to avoid getting blocked. Those jobs could also update the local html files.
Running without those short term is also fine since most API changes are breaking the extractor. Meaning the NewPipe repo will get bug reports depending on the feature (YT decrypt function, YT comments)

With this the unit tests are covering regressions and the smoke tests are covering whether the feature is actually working.

Thoughts?


In short:

    • Adjust architecture to allow injecting html into extractor Missed that its already in place
    • Save websites needed for tests as html locally Mock mix pl tests #482
      Functionality is implemented. See YoutubeMixPlaylistExtractorTest for usage
    • Implement smoke tests Existing tests can be used with my approach.
    • Mark flaky tests
@Stypox
Copy link
Member

Stypox commented Dec 13, 2020

I already did some efforts a while ago at mocking extractor tests with a HAR on the har-mock branch on my fork, maybe it can be useful: https://github.com/Stypox/NewPipeExtractor/tree/har-mock

@XiangRongLin
Copy link
Collaborator Author

@Stypox
Definetly interesting to see. Especially that a thing such as har exists.
For most har would be overkill i think, but there are some things where headers&co are important where that would be needed.

Do you have plans to get that merged or should i just strip out stuff that i want?

@Stypox
Copy link
Member

Stypox commented Dec 14, 2020

That branch was just used to process HARs provided by users, so it is not meant as a real testing strategy. So feel free to take snippets of code from it ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants