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

Add database test for SubscriptionManager #8456

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Add database test for SubscriptionManager #8456

merged 2 commits into from
Jul 18, 2023

Conversation

SydneyDrone
Copy link
Contributor

@SydneyDrone SydneyDrone commented May 29, 2022

What is it?

  • Codebase improvement (dev facing)

Description of the changes in your PR

  • A JUnit test file is added to test SubscriptionManager.kt (see New streams notifications #2335). However, the test result shows that either the original methods or my understanding of those methods has some problem, so I made this a draft.
  • The first problem is that the ChannelInfo I insert doesn't equal to the one I searched. The test result shows that it's because of their difference in the uid. IDK if it is designed to operate like that. If it is, I will delete the corresponding test cases in later commit.
  • The second problem is that the updateNotificationMode() method doesn't seems to function correctly. The testUpdateNotificationMode() method 2 and 4 seems to be problematic and redundant, but I will temporarily keep them until this problem is solved or clairfied.
  • The third problem is that the rememberAllStreams() method doesn't seems to fetch all videos as it should be (in my test case, it just fetched two instead of the total number of 100+)

Before/After Screenshots/Screen Record

  • The total test result:
    image

Fixes the following issue(s)

Due diligence

@SydneyDrone SydneyDrone changed the title Add database test for SubscriptionManager (review needed) Add database test for SubscriptionManager Jun 1, 2022
@opusforlife2
Copy link
Collaborator

Are you adding all possible tests that involve the database? If not, then I don't think you should link this PR to that issue. This would then be one of several PRs that work towards completion of the objective stated there.

@opusforlife2
Copy link
Collaborator

@SydneyDrone Please try to provide a reason when you close a PR. It helps when we're looking at it in the future.

@Stypox
Copy link
Member

Stypox commented Jul 6, 2022

@SydneyDrone thank you for writing these tests, I think they are valid. Could you reopen?

@SydneyDrone SydneyDrone reopened this Jul 19, 2022
@SydneyDrone
Copy link
Contributor Author

@SydneyDrone thank you for writing these tests, I think they are valid. Could you reopen?

It's reopened now. Sorry for the inconvenience.

@Stypox
Copy link
Member

Stypox commented Jul 20, 2022

I made some changes:

  • fix checkstyle errors
  • tests do not run in order, so each one has to do its own assertions separately from what others did
  • the uid of an entity in the database needn't be the same of the one created in-memory, since the uid gets assigned upon inserting in the database
  • some database functions return a Completable that doesn't do anything until it is subscribed to or awaited, so I added .awaitBlocking() where needed
  • the data of an entity in-memory does not get updated automatically when the corresponding entity in the database is changed, so some tests have been removed
  • manager.insertSubscription only inserts recent streams, so they need to have a date set on them (I also made related items hardcoded and not dependent on what the channel is currently doing)

Now the tests pass and I think they are reasonable. Tell me what you think ;-)

@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@SameenAhnaf SameenAhnaf added codequality Improvements to the codebase to improve the code quality database Issue is related to database operations labels May 13, 2023
SydneyDrone and others added 2 commits July 18, 2023 08:36
- fix checkstyle errors
- tests do not run in order, so each one has to do its own assertions separately from what others did
- the uid of an entity in the database needn't be the same of the one created in-memory, since the uid gets assigned upon inserting in the database
- some database functions return a `Completable` that doesn't do anything until it is subscribed to or awaited, so I added `.awaitBlocking()` where needed
- the data of an entity in-memory does not get updated automatically when the corresponding entity in the database is changed, so some tests have been removed
- `manager.insertSubscription` only inserts recent streams, so they need to have a date set on them (I also made related items hardcoded and not dependent on what the channel is currently doing)
@TobiGr TobiGr marked this pull request as ready for review July 18, 2023 06:40
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@TobiGr TobiGr merged commit 5706447 into TeamNewPipe:dev Jul 18, 2023
This was referenced Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality database Issue is related to database operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more tests for methods involving the database
5 participants