-
Notifications
You must be signed in to change notification settings - Fork 621
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
Feature/issue 38 restart period #67
Conversation
testRestartPeriodStreamFetcher
//wait 10-12 seconds | ||
Thread.sleep(13000); | ||
|
||
//check that stream fetcher stop and start stream is not called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a separate test for each verifying a feature. Ex: each verify after Thread.sleeps look like testing a different thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a kind of scenario testing. There is periodic task in StreamFetcher, we change some metrics on the fly and verify periodic task is working properly. Change again some metrics and verify test case again. This is real use case scenario. We need to sleep sometime to make sure the periodic task do its job.
this.schedulingService = schedulingService; | ||
this.datastore = datastore; | ||
this.scope=scope; | ||
if (streamFetcherFactory != null) { | ||
this.streamFetcherFactory = streamFetcherFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make it more readable, this might be an option:
this.streamFetcherFactory = streamFetcherFactory;
if(this.streamFetcherFactory == null){
this.streamFetcherFactory = new StreamFetcherFactory();}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check this out
e3f6052
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mekya add this also this.streamFetcherFactory = streamFetcherFactory;
@@ -68,10 +113,12 @@ public Result startStreaming(Broadcast broadcast) { | |||
Result result=new Result(false); | |||
|
|||
try { | |||
StreamFetcher streamScheduler = new StreamFetcher(broadcast,scope); | |||
StreamFetcher streamScheduler = streamFetcherFactory.make(broadcast, scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making StreamFetcherFactory a static class, make "make" method a static method, so there will be no need to create an instance and keep as variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making make method static is ok. On the other hand, in testing we give a mock StreamFetcherFactory instance as a parameter to create a test case. How can we do that not using an instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StreamFetcherFactory class has only "make" method. You dont need an instance to call it if you make the "make" method static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Please just take a look at here
https://github.com/ant-media/Ant-Media-Server/blob/dev/src/test/java/io/antmedia/test/StreamFetcherUnitTest.java#L258
We give a different mock StreamFactory class. If we call StreamFactory.make static function directly how we give a separate mock in the test case above? Maybe we need to change some other things in the code as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will not give a mock and you will not pass as parameter.
Pull Request
This PR fixes #Enter issue number
Changes proposed in this pull request: