Conversation
alan-george-lk
changed the title
stephen-derosa
left a comment
stephen-derosa
left a comment
There was a problem hiding this comment.
Looks good to me thanks for doing this!
| - os: ubuntu-latest | ||
| name: linux-x64 | ||
| build_cmd: ./build.sh release-examples | ||
| build_cmd: ./build.sh release-tests |
There was a problem hiding this comment.
should we be using build-all here? note it will add significant time since it builds debug/release tests/exampls
There was a problem hiding this comment.
Open to it, I can profile that. Would be good to build all but yeah, will increase time
| fi | ||
| return 1 | ||
| } | ||
| for pair in "SimpleRoom:simple_room" "SimpleRpc:simple_rpc" "SimpleDataStream:simple_data_stream"; do |
There was a problem hiding this comment.
is it safe to completely remove the smoke tests? Does it make sense to keeo them to ensure that they are compiled and be executed?
There was a problem hiding this comment.
The smoke tests were failing to build early in this endeavor, that might have been before I split the unit tests out. Can put this back, let me review and get back to you
| & "${{ matrix.build_dir }}/bin/livekit_unit_tests.exe" ` | ||
| --gtest_output=xml:${{ matrix.build_dir }}/unit-test-results.xml | ||
|
|
||
| - name: Upload test results |
| int sample_rate = 0; | ||
| int num_channels = 0; | ||
|
|
||
| // TODO: figure out what generates this welcome.wav file such that this test isn't skipped |
There was a problem hiding this comment.
please create a ticket for this and use TODO(project-ticketid). I am guilt of not doing this as much as i should.
There was a problem hiding this comment.
This was a TODO for me before this PR, ideally I'll fix it before we merge, otherwise 100% will do
| int sample_rate = 0; | ||
| int num_channels = 0; | ||
|
|
||
| // TODO: figure out what generates this welcome.wav file such that this test isn't skipped |
There was a problem hiding this comment.
It was something very basic, I generated it by using the the say cmd on macbook, like
say "Hello world" -o hello_world.wav
| - os: macos-latest | ||
| name: macos-arm64 | ||
| build_cmd: ./build.sh release-examples | ||
| build_cmd: ./build.sh release-tests |
There was a problem hiding this comment.
curiously, should we build all instead ?
like
./build.sh release-all
| shell: pwsh | ||
| run: ${{ matrix.build_cmd }} | ||
|
|
||
| # ---------- Smoke test cpp-example-collection binaries ---------- |
There was a problem hiding this comment.
I would assume it is still useful to run this cpp-example-collection ?
If any change is required inside the cpp-example-collection, that raise a flag that we might have a breaking API change ?
| # ============================================================================ | ||
|
|
||
| file(GLOB UNIT_TEST_SOURCES | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/unit/*.cpp" |
There was a problem hiding this comment.
nit, generally it is better to add all the cpp files explicitly to a list.
For instance, with this current code, when adding new test files, CMake won't automatically rerun.
| ${Protobuf_INCLUDE_DIRS} | ||
| ) | ||
| if(TARGET absl::base) | ||
| get_target_property(_livekit_unit_test_absl_inc absl::base INTERFACE_INCLUDE_DIRECTORIES) |
There was a problem hiding this comment.
This smells a bit.
Why do we need such special handling of absl::base ?
I wonder if we have some real problem with our SDK, technically, consumers of livekit SDK needs to manually have the include dirs from absl::base unless the tests use them ?
| ${LIVEKIT_ROOT_DIR}/include | ||
| ${LIVEKIT_ROOT_DIR}/src | ||
| ${LIVEKIT_BINARY_DIR}/generated | ||
| ${Protobuf_INCLUDE_DIRS} |
There was a problem hiding this comment.
why protobuf is needed ?
maybe some tests are not correctly written ?
3 participants
This PR adjusts the CI to do the following:
release-testsintegration/existed in the tests directory, but contained unit-level testing. A new folderunit/was added, which is now being executed in CINote: part of the time spent on this PR was spent investigating a separate testing stage for unit testing (and upcoming
clang-tidy), but passing artifacts between stages became pretty cumbersome and affected overall build usefulness. I think a future refactor of the CI jobs should be considered to improve this.