[Soundcloud] Initial soundcloud test refactoring#1324
[Soundcloud] Initial soundcloud test refactoring#1324absurdlylongusername wants to merge 9 commits intoTeamNewPipe:devfrom
Conversation
… class tests not being executed
…eamLinkHandlerFactory Use non-capturing groups in regex Refactor Parser.java Add more utility methods (will be used in later commits)
absurdlylongusername
changed the title
TobiGr
added
SoundCloud
…om Immutables test case object Add Immutables annotation processor to gradle build Add custom Immutables Style Add ISoundcloudStreamExtractorTestCase for use in stream extractor tests Refactor testAudioStreams to match mp3 cdn url by regex
… API <26) Add javadocs to other methods in Parser.java
absurdlylongusername
force-pushed
the
initial-soundcloud-test-refactoring
branch
from
cad1c80 to
b89ac78
Compare
There was a problem hiding this comment.
Thank you for this and sorry for the late review. While I do like the builder pattern better than test subclasses, I don't think it's worth to spend a lot of time to refactor all of the tests in the extractor to this pattern, for these reasons:
- While the current test infrastructure might be a bit more verbose and/or have more boilerplate, it is also more flexible and easy to debug, as there is one less layer of abstraction that tests have to go through.
- In most tests we just need to provide data to common testing methods that behave always the same. In these cases the builder pattern is the best solution. But in many cases throughout the extractor we have custom logic specific to a specific test: e.g. custom initialization of the
extractorfield, custom tests just for some cases, etc. I'm not saying there is no way to add specific logic with the builder pattern, but it would again not be super elegant. - The current way tests are setup might not be the best possible, but it has worked just fine so far, is easily extensible and involves only little boilerplate (you just need one line of code for each piece of data you want to specify, just like with the builder pattern, although there's less stuff around it).
I understand what you are trying to achieve with this PR and I see why you don't like how tests are setup right now. My points above are not meant to be in support of the status quo as the best solution, but rather try to argue why the current tests are already good enough and we wouldn't get much more value by refactoring them. Refactoring everything would take a lot of time, and given how repetitive it would be as a task, it will also be easy to make mistakes. I think that the value we would gain from refactoring to the builder pattern (in terms of better readability, easier testing, etc.) is very small.
What do other team members think? @TobiGr or @AudricV or @litetex? Also, in case we still decide to move forward, this will need a rebase.
|
@Stypox In hindsight, yes I do agree it's not worth doing this atm purely due to the fact it is not a priority right now, not absolutely necessary, and a lot of this test code is not being used/developed around that much. That being said, I believe that tests and stuff that we add in the future should use a better pattern rather than the patterns that we use right now. E.g. #1360 would require refactoring a lot of stuff around Soundcloud, and in turn would mean changing some tests and adding new tests. Ideally, I'd want these new tests to be made in a way where it's not a class per test case. Would you agree? Also this kind of begs the question: how often do we need tests that have many test cases? Because we do indeed have some tests in our codebase that are structured like this and have several test cases (e.g. SoundcloudSteamLinkHandlerFactoryTest). IIRC, my main rationale for wanting to do this refactor was that all tests should be extensible (in the dimension of being able to add more test cases) by default (i.e. it is the same test with multiple test cases, not a new test with @test annotation); but, if, realistically, most of the tests will not require this functionality then it's not much of a reward doing this refactor everywhere. If tests with many test cases was a requirement or design concern, then the current test infrastructure that I want to refactor wouldn't have been designed the way it is. So, it makes sense for me to conclude that for most or all of the tests that have this current structure do not really require many test cases in order to be tested approriately. So, practically, imo, if anything is going to be refactored it should only be done if the tests actually need to be expanded in the dimension of needing many test cases for tests, OR if that part of the code is already going to undergo a large enough structural change that refactoring it in the process can easily be done in parallel without much overhead. Given this, this PR doesn't meet either of these criteria so it is not necessary and therefore can be abandoned. |
I would say that for remote content (videos, playlists, etc.) we don't usually need tests with many test cases:
I think that for link handlers it makes a lot of sense to have test cases, because the link handler functions to test often have just a couple of inputs and outputs, and always the same amount thereof. Videos on the other hand have a lot of fields that should just default to some value, and we'd need the builder pattern to achieve something like that, which is already more verbose than just a list of test cases. But yeah we should definitely use test case lists (I don't know how to call it, maybe it's "instanced tests"?) for link handlers and other input->output functions.
This sounds like a good plan :-) |
3 participants
Add test cases for SoundcloudStreamExtractorTest
This PR makes initial progress on refactoring testing infrastructure to use test cases to generate tests instead of current architecture that uses a class to define each test case.
It uses Immutables to define test cases.
ISoundcloudStreamExtractorTestCaseis the test case used for SoundcloudStreamExtractorTest, which inherits from other default extractor test cases.In future PRs this will be expanded on to further decouple the current test format so we can have tests that can run multiple test cases instead of defining a new class for each test case. This will make tests easier to change, more versatile and extensible.
Update tests in SoundcloudStreamExtractorTest
Changed how the tests for SoundCloud CDN urls worked to use a regex instead of matching the ID explicitly.
Please Note
This PR includes changes from #1323 which couldn't be removed as it depends on it. However, since those changes don't depend on this, I have included them in that other PR so they can be merged first before this one.