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

IS-05: Testing scenario when two receivers get bulk-staged for relati… #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bakaleks
Copy link

@bakaleks bakaleks commented Jan 9, 2019

…ve scheduled activation. Both of them should be activated. Testing with Sony's nmos-cpp-node sample application with two registered receivers

…ve scheduled activation. Both of them should be activated. Testing with Sony's nmos-cpp-node sample application with two registered receivers
@bakaleks
Copy link
Author

bakaleks commented Jan 9, 2019

Testing with following change sony/nmos-cpp#43

Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

Thanks, @bakaleks. I think this will be a great addition to the test suite - it demonstrated a bug in my implementation as you said.

I'd like to see a bit of refactoring to eliminate the duplication with IS05Utils.check_perform_relative_activation if possible. What do you think, @andrewbonney?

@andrewbonney
Copy link
Contributor

I'll ask @simonrankine to take a quick scan. Otherwise as this is your first contribution @bakaleks we'll probably have to ask you to fill in an IPR form as-per point 3 in https://github.com/AMWA-TV/nmos-testing/blob/master/CONTRIBUTING.pdf. It might be easiest for us to discuss in person next week.

@simonrankine
Copy link
Contributor

Over-all this looks like a good and useful thing to have in the test suite, thanks very much for your contribution. What I would say is that there seems to be a lot with duplication with check_bulk_stage in the same file, to the point where I think I'd like it to be combined with check_bulk_active_relative before we merge, or this code is going to get mighty hard to maintain.

@andrewbonney andrewbonney added the pending refactor PR has been reviewed and requires some modifications before merging label Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending refactor PR has been reviewed and requires some modifications before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants