roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.06k stars 213 forks source link

Improve integration tests for public API #328

Closed gavv closed 1 year ago

gavv commented 4 years ago

Currently we have 3 integrations tests for our C API:

We need to cover more parameter combinations:

Before adding more tests, it would be also nice to extract Context, Sender, Receiver, and Proxy from test_sender_receiver.cpp into separate files in src/tests/roc_library (i.e. test_context.h, test_sender.h, and so on).

This work can be split into multiple PRs, one or a few tests per PR.

CleverShovel commented 4 years ago

I would like to try write these tests.

gavv commented 4 years ago

@CleverShovel Great, you're welcome!

CleverShovel commented 4 years ago

@gavv Is this normal that in roc_library directory there are files like test_context.cpp, test_sender.cpp and others and I will add test_context.h and others but they will not related to these cpp?

gavv commented 4 years ago

Good catch. Since this issue was created, the file layout in tests was changed a bit. According to the new layout, these files should be named like roc_library/test_helpers/context.h, roc_library/test_helpers/sender.h, etc. See, for example, tests for roc_pipeline or roc_audio.

CleverShovel commented 4 years ago

@gavv Does each item from the list in issue description mean concrete tests or only a parameter to combine?

gavv commented 4 years ago

Well, it's just a list of features to be covered somehow. Obviously, we can't create a test for every possible combination of these features because there were too much tests. So, likely, we should create one or a few test for every specific item. E.g., for Enable or disable packet interleaving we can create one test covering both cases or two separate tests, one w/o interleaving, one w/ interleaving. That's not a strict requirement, though. For example, if we already have another test covering transfer w/o interleaving, there is no need to duplicate it.

CleverShovel commented 4 years ago

@gavv As far as I can see for parallel test of multiple senders and one receiver I should add another one roc_endpoint and roc_receiver in Receiver class and additionally adjust while cycle in method Receiver::run(). Am I right?

gavv commented 4 years ago

You can connect multiple senders to the same endpoint(s). The receiver will mix them. You need to adjust the received stream checking to expect the stream mixed from multiple senders.

The hard thing here is that you don't known how the streams from different senders are shifted. One possible way to deal with it is to make the first sender produce stream like 10, 20, 30, ..., and the second sender 1, 2, 3, ... Then you'll read from the receiver something like 25, 36, 47, ... and from that you can deduce the original sequences of both senders and check them separately.

gavv commented 4 years ago

multiple_senders_one_receiver_sequential is failing quite often, I disabled it for now.

https://travis-ci.org/github/gavv/roc/jobs/671349254

fail2

https://travis-ci.org/github/gavv/roc/jobs/671349250

fail1

@CleverShovel Could you please take a look?

CleverShovel commented 4 years ago

@gavv ok, I'll check test more carefully

CleverShovel commented 4 years ago

@gavv When I debugged the test in Receiver::run() I got these data in rx_buff: Снимок экрана от 2020-04-06 21-47-09 Снимок экрана от 2020-04-06 21-49-29 Receiver expect data in range (0, 1) but there can be very small numbers as well as very big numbers and nan. Can I add some function that check that received number in range (0, 1) ? Also I think that variable wait_for_signal not very useful here. One time the tests were failed because of this variable. When in prev_sample number from the previous frame and wait_for_signal = true value of prev_sample isn't set to beginning of current frame. Unfortunately reproduce this error I can't.

gavv commented 4 years ago

Receiver expect data in range (0, 1) but there can be very small numbers as well as very big numbers and nan.

This looks very strange. If receiver produces samples out of range [0; 1] [-1; 1], it's a bug. But are you sure that you're debugging it correctly? How did you get the numbers on screenshot?

Can I add some function that check that received number in range (0, 1) ?

Yes, adding such a check is legit and makes sense. But the range is [-1; 1].

Also I think that variable wait_for_signal not very useful here.

IIRC it was added for reason. Maybe @alexandremgo can provide some details on this.

gavv commented 4 years ago

I think the problem with multiple_senders_one_receiver_sequential is that we're not waiting until the receiver will fully process the samples from the first sender before starting the second one.

I guess we should change the flow to the following:

gavv commented 4 years ago

BTW, note that this test likely will work incorrectly under a debugger because of its real-time nature. (Though receiver should not produce numbers out of [-1; 1] range anyway).

gavv commented 4 years ago

An interesting detail: both failures on travis were on sample #3999.

gavv commented 4 years ago

@CleverShovel Hi, do you have any further plans on this?

CleverShovel commented 4 years ago

@gavv Sorry for long delay, I'm was busy. I try to find time for this by the end of this month.

gavv commented 4 years ago

Great!

gavv commented 4 years ago

Unassigning so that someone can continue the work during hacktoberfest. @CleverShovel feel free to let me know if you want to be re-assigned though.

gavv commented 1 year ago

I've created separate issues for remaining tests: