ryanheise / audio_service

Flutter plugin to play audio in the background while the screen is off.
803 stars 480 forks source link

Update unit tests for one-isolate branch #716

Open ryanheise opened 3 years ago

ryanheise commented 3 years ago

Is your feature request related to a problem? Please describe.

The unit tests are not in working order on the one-isolate branch.

Describe the solution you'd like

Migrate the unit tests to work with the new audio_service API.

Describe alternatives you've considered

None

Additional context

None

suragch commented 3 years ago

This is something I might be able to help with. If you would like help implementing the tests, are there any special instructions?

Note to self: link to tests

ryanheise commented 3 years ago

Sure! You are welcome to take a stab at it while I take on another task. Now that we have a platform interface, you ought to be able to mock that with a mock implementation of that interface or mockito instead of trying to mock the method channel. Let me know if you have any questions.

nt4f04uNd commented 3 years ago

(update): today-tomorrow will end the _IsolateAudioHandler and add a bunch of integration tests for it with flutter_isolate, or unit, if that's feasible, will see

suragch commented 3 years ago

Ok, so I'm looking at this today and I guess I don't really know how to start. I would need a walk-through but by that time one of you might be able to finish. I'm still willing to try, but I'd probably at least need a more detailed list of steps to follow.

nt4f04uNd commented 3 years ago

Synchronizing subjects between isolates is much harder task than I first thought initally.. What I currenly have is that I tried on both ends to listen to both stream and port that receives messages from the other isolate, but this obviously leads to that messages are sent back and forth infinitely by this scheme:

  1. subject has listener that sends message to other isolate
  2. in that isolate message is added into the subject
  3. because this subject is also listened the event is send back to the sender isolate
  4. isolate receives event and calls add
  5. back to 1

Any thoughts?

nt4f04uNd commented 3 years ago

Tests for the isolate are mostly done, except for subjects

nt4f04uNd commented 3 years ago

Ah, that's easy, I will just filter these messages out lol

nt4f04uNd commented 3 years ago

@suragch I will give you some steps a bit later, once I'm done with what I'm currently doing, since this will give a ground for other tests, such as MockBaseAudioHandler

nt4f04uNd commented 3 years ago

@suragch I pushed my changes in https://github.com/ryanheise/audio_service/pull/735, you can check them out for example, I'm testing IsolateAudioHandler there. Other unit tests will require writing something similar, but with other handlers.

You can also checkout https://github.com/ryanheise/audio_service/pull/726 if you want

Here are some sections we will need to cover

Platform connection

I think that we will need to expose the _HandlerCallbacks for testing a communication with AudioServicePlatform. Tests would go like this:

  1. At the very beginning we create our newly exposed AudioHandlerCallbacks class and create a MockBaseAudioHandler
  2. Then for each method of the callbacks we call it with specific parameters and expect the MockBaseAudioHandler to be called also with specific parameters and return a specific value, if it can

Other handlers

Data classes

Classes like MediaItem, Rating, PlaybackState, etc. - ensure they are translated into platform messages and they generally work correctly

I think you should still wait a little bit though, until @ryanheise can review my PRs

suragch commented 3 years ago

@nt4f04uNd Thank you for your outline. That was really helpful.

I started some work on this. I added tests for BaseAudioHandler. Would someone mind taking a look and telling me if I'm on the right track:

ryanheise commented 3 years ago

Awesome @suragch ! Can you create a draft pull request? I can then use GitHub's facilities to do a review.

nt4f04uNd commented 3 years ago

@ryanheise I'm planning to test the data classes. Do you think we should add tests for enums and enum-like classes as well?

ryanheise commented 3 years ago

All we really need is to test each method has the expected outcome - enums themselves don't have methods, so I think their coverage would be incidental rather than the focus of specific unit tests.

suragch commented 3 years ago

@nt4f04uNd If you are testing the data classes, one thing to note is PlaybackState.copyWith is missing the updateTime parameter.

nt4f04uNd commented 3 years ago

I also have a platform connection tests written locally which are based of https://github.com/ryanheise/audio_service/pull/735

suragch commented 3 years ago

I meant to keep working on this but got busy with work. What is the current status of the unit test updates? Is there something I can do?

ryanheise commented 3 years ago

Hi @suragch , thanks so much for your previous work on this which has now been merged.

Full coverage is not ultra urgent, as I would be happy to include the current level of testing in the next stable release and then chip away at it after that.

But if you're interested to know what's left to be done, you can run flutter test --coverage to generate the coverage data, and then use a visualisation tool on the generated coverage data to reveal which lines of code may not have been tested yet. Currently we're at 47.8% coverage, and long term I'd like to get this above 70% coverage