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

Added.cpp and.h files for the command line tool test, modified CMakeLists.txt #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

u7763283
Copy link

The test content can run normally and the test results are correct.

Sage Rothschild and others added 3 commits October 27, 2023 23:08
@u7763283
Copy link
Author

Reference issues:
#4
roc-toolkit:Issue #552

@gavv
Copy link
Member

gavv commented Nov 2, 2023

Thanks for PR, will review it in upcoming days!

@gavv
Copy link
Member

gavv commented Nov 6, 2023

Nice! Here are a few suggestions:

  1. Since we use libsndfile for reading wav files, I guess we can use it in generate_wavfile() too, to simplify the code.

  2. For WAV file, it's better to use temporary file instead of using hard-coded path in current dir. This way we don't limit concurrent execution of same tests, don't touch source directory, and don't require current dir to be writable by current user (may be important when we execute tests on remote box). I guess yoy could use std::filesystem::temp_directory_path().

  3. We can use sndfile.hh instead of sndfile.h, it provides C++ version of the library with RAII (we won't need to call sf_close() manually).

  4. Let's use vector of strings instead of single string for commands (like, {"roc-send", "-vv", ...} instead of "roc-send -vv ..."). This way we can be sure we won't mess with shell substitutions (e.g. we don't need to escape temp file path).

  5. Please rename file from command_line.cpp to test_command_line.cpp (same as the other test is named). Also I think the header file is not needed because it's never used outside the test. You can move class definition to an anonymous namespace in the .cpp file.

  6. On my machine, test fails:

    [ RUN      ] CommandLine.SendReceiveWavFile
    Successfully open!
    Wav file sent successfully!
    
    Wav file received successfully!
    0@
    Samplerate does not match
    sended file:44100
    received file:48000
    /home/victor/dev/roc-streaming/rt-tests/tests/command_line.cpp:185: Failure
    Value of: compare_wavfiles(filename.c_str(), received_file.c_str())
      Actual: false
    Expected: true
    The contents of the original file and the received file do not match!
    [  FAILED  ] CommandLine.SendReceiveWavFile (1511 ms)
    

    It can be fixed by adding --rate 44100 to receiver command-line. Please add it.

  7. On my machine, there is a segfault in compare_wavfiles(). The problem is that sf_readf_double() expects number of samples per channel (a.k.a. number of frames), so that you need to pass to it buffer_size / 2 insead of buffer_size.

  8. After fixing two problems above, the comparison fails for me:

    [ RUN      ] CommandLine.SendReceiveWavFile
    Successfully open!
    Wav file sent successfully!
    
    Wav file received successfully!
    
    File data do not match
    /home/victor/dev/roc-streaming/rt-tests/tests/command_line.cpp:185: Failure
    Value of: compare_wavfiles(filename.c_str(), received_file.c_str())
      Actual: false
    Expected: true
    The contents of the original file and the received file do not match!
    [  FAILED  ] CommandLine.SendReceiveWavFile (1512 ms)
    

    Actually I expected this from the code. The reason is that you're doing direct comparison in compare_wavfiles(). Doing direct comparison is not correct - the received stream may differ from the sent stream: there may be shifts, drops, and leading silence.

    This is described in detail here: Improve acceptance criteria of end-to-end test roc-go#101. The last section (starting with "Here is an algorithm ...") describes how we can do the comparison correctly. This algorithm is implemented in improve e2e test roc-go#111. It's in Go, not in C, but I think it's rather readable even if you don't know Go. In rt-tests, we should implement something similar.


BTW, in case you're interested, later you could take a look at these issues in toolkit repo, I think they would have implementation similar to what you did here: roc-streaming/roc-toolkit#576, roc-streaming/roc-toolkit#246.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants