Refactor SoundCloudStreamExtractorTest to use test cases and fix failing tests

[absurdlylongusername]

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

Validate HTTP response codes for Soundcloud

Production code that was using Downloader does not do any checking on the response code: therefore errors and exceptions thrown due to error response code give false information (e.g. tests failing due to 404 were giving error about clientId when the error was 404).

I added method in Response.java and a utility method for validating response codes and used it in places for soundcloud extraction. It will throw exception with information about the error code, which will propagate upwards to the call site which will print a more useful stack trace for debugging.

Add test cases for SoundcloudStreamExtractorTest

This PR makes initial progress on refactoring extractor tests 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. ISoundcloudStreamExtractorTestCase is the test case used for SoundcloudStreamExtractorTest, which inherits from other default extractor test cases.

In future PRs I will add more refactoring to further decouple the current test infrastructure from expected test data 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.

Fixed failing tests in SoundcloudStreamExtractorTest

Tests were failing due to 404. Fixed by adding different track and some other tracks as well

Fixed failing tests in SoundcloudChannelTabExtractorTest

Refactor Parser.java

Refactored regex utility classes to remove DRY. Add javadocs

Minor refactoring

Used List.of in a few places because IDE said so

Use Pattern in SoundcloudStreamLinkHandlerFactory instead of String

Refactor AudioStream to use builder in constructor

Make constructor take in builder instead of telescopic list of parameters

SoundcloudStreamExtractor.getTimeStamp return 0 if no timestamp found

Added this change to be in line with other extractors: I think this should be expected behaviour and would produce bugs otherwise

[litetex]

Thank you for the PR but I think this is way to huge and needs to be broken down into:

• fixing tests
• adding response code validation
• Introducing a new way to test (for like 5 classes)
• Introducing new dependencies
• Doing minor code cleanup
• Using var

And for half of these things it needs to be checked if these are required in the first place...

There is also currently a redesign of the overall API in progress and I think this would cause conflicts...

[absurdlylongusername]

@litetex 😅Funny you say that, because this was already the result of me breaking down the PR lol.

Fixing tests

• fixing tests
• response code validation

The response code validation and fixing tests are kind of related because tests were failing due to 404 and only after figuring that out was I able to fix the test, so I think it should be allowed for both of those to be in a single PR since they are logically related.

New way to test

• Introducing a new way to test (for like 5 classes)

Do you mean I should extend the refactoring to 5 other classes instead of just with SoundcloudStreamExtractorTest?

• Introducing new dependencies

Quite obviously, the new way to test cannot be separated from using immutables, and to be honest I don't think it makes sense having a single PR just to add a dependency if it the dependency is not being used in the same PR. I believe every PR should be functional and should not add dead code.

Doing minor code cleanup

I can move some of this to a separate PR, but some of the changes are logically related to other changes, but also if I break up this PR further, I think as a result, since the minor code cleanup is not actually that much, I don't think it would need to be in a separate PR anymore and can just be included in any of the other PRs that are small.

I am of the opinion that, up to a certain point, any PR should be allowed to include minor code cleanup in order to make it easier to improve code quality of a codebase without having to make an explicit refactoring PR every time. Especially in a very old codebase such as this. Reducing refactoring overhead makes refactoring easier.

• using var

Why would this need be a separate PR by itself? I think it should just constitute minor code cleanup.

There is also currently a redesign of the overall API in progress and I think this would cause conflicts...

I am not currently aware of any redesign of the extractor that is ongoing, I am only aware of the rewrite which is primarily refactoring of NewPipe, and not NewPipeExtractor. Please correct me if I am wrong.

And for half of these things it needs to be checked if these are required in the first place...

Required meaning what exactly?

Most of the extractor code hasn't been touched in many years, so one might argue "if it ain't broke don't fix it", but I don't necessarily see any downside in improving the code quality.

[absurdlylongusername]

@litetex Update: I've broken this down further into three PRs. I will link them here then close this PR

[absurdlylongusername]

Closing in favour of:

• Validate HTTP response codes across the extractor  #1322
• [SoundCloud] Miscellaneous refactors #1323
• [Soundcloud] Initial soundcloud test refactoring #1324