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

Extract settings import #5225

Merged
merged 10 commits into from Jan 14, 2021
Merged

Conversation

XiangRongLin
Copy link
Collaborator

@XiangRongLin XiangRongLin commented Dec 19, 2020

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

  • Extract logic for settings import out of ContentSettingsFragment into ContentSettingsManager and ZipHelper
  • Add basic unit tests for ContentSettingsManager
  • Introduce NewPipeFileLocator which resolves the db and settings files. Inject it into ContentSettingsManager
  • remove subclass ExportTest. No benefit with only one test
  • added real zip file and newpipe.settings of export as test resources

APK testing

app-debug.zip

https://github.com/TeamNewPipe/NewPipe/actions/runs/459642451

Due diligence

Notes

  • The git history went wide, because while extracting it I found my initial approach for DI and testing not good. Easiest would be to just squash everything together.
  • With this changes to the import/export logic should always be covered by additional unit tests

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. Thank you!

@Stypox Stypox merged commit 10c35f3 into TeamNewPipe:dev Jan 14, 2021
@XiangRongLin XiangRongLin deleted the extract_settings_import branch January 14, 2021 14:22
@Stypox
Copy link
Member

Stypox commented Jan 14, 2021

@XiangRongLin I am unable to run ContentSettingsManagerTest on my machine. I am unable to debug #5415 tests.

Cannot mock/spy class org.schabi.newpipe.settings.NewPipeFileLocator
Mockito cannot mock/spy because :
 - final class

If I make NewPipeFileLocator an open class (in kotlin classes are final by default), I get null pointer exceptions whenever something is being mocked, like 'when'(fileLocator.db).thenReturn(db).

@Stypox Stypox mentioned this pull request Jan 14, 2021
14 tasks
@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Jan 15, 2021

@Stypox Did you sync the Gradle files? Because the mockito-inline dependency should fix the problem with mocking final classes/methods

@XiangRongLin
Copy link
Collaborator Author

@Stypox the NullPointerExceptionoccurs for the same reason. Methods are final by default, so mockito can't mock then out of the box.

This was referenced Jan 18, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants