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 export database logic into own class #5059

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

XiangRongLin
Copy link
Collaborator

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

  • Separate database/settings generation logic from the UI.
  • Add happy path unit test.

With this seperation, unit tests can be introduced. I just added one as proof of concept for now.
If this style is ok i would work on extracting more stuff out.

This also allows cheap unit tests instead of comparatively expensive androidTests.

APK testing

app-debug.zip

Due diligence

- Separate it from the UI.
- Add happy path unit test.
@XiangRongLin
Copy link
Collaborator Author

I added the setting Mockito.withSettings().stubOnly() in the test, since a stub is enough

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.

We definitely need more tests, so I like this. I tested on my device and it works; I also run the test locally and it works. Thank you!

@Stypox Stypox merged commit d46c7eb into TeamNewPipe:dev Dec 15, 2020
@XiangRongLin XiangRongLin deleted the content_settings_manager branch December 15, 2020 10:19
private lateinit var newpipeDb: File
private lateinit var newpipeSettings: File

@Before
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well from refactoring too much stuff around, i apparently created a beforeClass method annotated @Before. I'll fix this later on in my next PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a separate PR for this

@@ -131,6 +129,8 @@ public void onCreatePreferences(final Bundle savedInstanceState, final String ro
newpipeSettings = new File(homeDir + "/databases/newpipe.settings");
newpipeSettings.delete();

manager = new ContentSettingsManager(homeDir);
Copy link
Member

@TobiGr TobiGr Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That causes the build to fail. Can you fix that, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, how is this possible? I ran the tests successfully on my machine...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW, i see where this is coming from. PR coming

XiangRongLin added a commit to XiangRongLin/NewPipe that referenced this pull request Dec 15, 2020
TeamNewPipe#5176 changed `homeDir` from type `String` to `File`. TeamNewPipe#5059 was based on `homeDir` being a `String`. It was incorrectly auto-resolved by git.
XiangRongLin added a commit to XiangRongLin/NewPipe that referenced this pull request Dec 15, 2020
TeamNewPipe#5176 changed `homeDir` from type `String` to `File`. TeamNewPipe#5059 was based on `homeDir` being a `String`. It was incorrectly auto-resolved by git.
This was referenced Dec 21, 2020
spvkgn pushed a commit to spvkgn/NewPipe that referenced this pull request Aug 4, 2021
TeamNewPipe#5176 changed `homeDir` from type `String` to `File`. TeamNewPipe#5059 was based on `homeDir` being a `String`. It was incorrectly auto-resolved by git.
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.

3 participants