Skip to content

Running unit tests in CI #89

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.

Sign up for GitHub

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

Open
alan-george-lk wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Running unit tests in CI #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
df684d0
First pass at unit tests in CI
alan-george-lk Apr 9, 2026
a931038
Separate unit tests
alan-george-lk Apr 9, 2026
21bab19
Fix unit tests on all platforms
alan-george-lk Apr 9, 2026
5911b65
Move all gtest discovery to "PRE_TEST" preventing invocation automati…
alan-george-lk Apr 9, 2026
5716889
Try fixing test stages
alan-george-lk Apr 9, 2026
abbdba4
Move unit tests to unit folder
alan-george-lk Apr 9, 2026
7df6650
Separate quality stage
alan-george-lk Apr 9, 2026
a7e5f0f
unify test stage
alan-george-lk Apr 10, 2026
b98cb97
Possibly fix windows
alan-george-lk Apr 10, 2026
34efb22
Merge branch 'main' into feature/ci_unit_tests
alan-george-lk Apr 10, 2026
e3362e9
Use full sha on actions, fixes to windows build
alan-george-lk Apr 10, 2026
498ea01
Fix integration stage build on Windows
alan-george-lk Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 22 additions & 99 deletions .github/workflows/builds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,23 @@ jobs:
include:
- os: ubuntu-latest
name: linux-x64
build_cmd: ./build.sh release-examples
build_cmd: ./build.sh release-tests
Copy link
Copy Markdown
Contributor

@stephen-derosa stephen-derosa Apr 10, 2026

Choose a reason for hiding this comment

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

should we be using build-all here? note it will add significant time since it builds debug/release tests/exampls

Copy link
Copy Markdown
Contributor Author

@alan-george-lk alan-george-lk Apr 10, 2026

Choose a reason for hiding this comment

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

Open to it, I can profile that. Would be good to build all but yeah, will increase time

build_dir: build-release
- os: ubuntu-24.04-arm
name: linux-arm64
build_cmd: ./build.sh release-examples
build_cmd: ./build.sh release-tests
build_dir: build-release
- os: macos-latest
name: macos-arm64
build_cmd: ./build.sh release-examples
build_cmd: ./build.sh release-tests
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026

Choose a reason for hiding this comment

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

curiously, should we build all instead ?

like
./build.sh release-all

build_dir: build-release
- os: macos-latest
name: macos-x64
build_cmd: ./build.sh release-examples --macos-arch x86_64
build_cmd: ./build.sh release-tests --macos-arch x86_64
build_dir: build-release
- os: windows-latest
name: windows-x64
build_cmd: .\build.cmd release-examples
build_cmd: .\build.cmd release-tests
build_dir: build-release

name: Build (${{ matrix.name }})
Expand All @@ -88,7 +88,7 @@ jobs:
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1
with:
submodules: recursive
fetch-depth: 0
fetch-depth: 1

# ---------- vcpkg caching for Windows ----------
- name: Export GitHub Actions cache environment variables
Expand Down Expand Up @@ -181,105 +181,28 @@ jobs:
shell: pwsh
run: ${{ matrix.build_cmd }}

# ---------- Smoke test cpp-example-collection binaries ----------
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026

Choose a reason for hiding this comment

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

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 ?

# Built under cpp-example-collection-build/ (not build-dir/bin). Visual Studio
# multi-config places executables in per-target Release/ (or Debug/) subdirs.
- name: Smoke test examples (Unix)
# ---------- Run unit tests ----------
- name: Run unit tests (Unix)
if: runner.os != 'Windows'
shell: bash
run: |
set -x
failed=false
examples_base="${{ matrix.build_dir }}/cpp-example-collection-build"
resolve_exe() {
local dir="$1" name="$2"
local p="${examples_base}/${dir}/${name}"
if [[ -x "${p}" ]]; then
printf '%s' "${p}"
return 0
fi
p="${examples_base}/${dir}/Release/${name}"
if [[ -x "${p}" ]]; then
printf '%s' "${p}"
return 0
fi
return 1
}
for pair in "SimpleRoom:simple_room" "SimpleRpc:simple_rpc" "SimpleDataStream:simple_data_stream"; do
Copy link
Copy Markdown
Contributor

@stephen-derosa stephen-derosa Apr 10, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@alan-george-lk alan-george-lk Apr 10, 2026

Choose a reason for hiding this comment

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

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

exe="${pair%%:*}"
dir="${pair#*:}"
if ! exe_path="$(resolve_exe "${dir}" "${exe}")"; then
echo "ERROR: ${exe} not found under ${examples_base}/${dir}/"
failed=true
continue
fi
echo "Testing ${exe}..."
output=$("${exe_path}" --help 2>&1) || true
if [[ -z "${output}" ]]; then
echo "ERROR: ${exe} produced no output"
failed=true
else
echo "${output}"
echo "${exe} ran successfully"
fi
done
if [[ "$failed" == "true" ]]; then exit 1; fi

- name: Smoke test examples (Windows)
${{ matrix.build_dir }}/bin/livekit_unit_tests \
--gtest_output=xml:${{ matrix.build_dir }}/unit-test-results.xml

- name: Run unit tests (Windows)
if: runner.os == 'Windows'
shell: pwsh
run: |
$ErrorActionPreference = 'Continue'
$examplesBase = "${{ matrix.build_dir }}/cpp-example-collection-build"
$examples = @(
@{ Name = 'SimpleRoom'; Dir = 'simple_room' },
@{ Name = 'SimpleRpc'; Dir = 'simple_rpc' },
@{ Name = 'SimpleDataStream'; Dir = 'simple_data_stream' }
)
$failed = $false
foreach ($ex in $examples) {
$name = $ex.Name
$dir = $ex.Dir
$inDir = Join-Path $examplesBase $dir
$candidates = @(
(Join-Path $inDir "$name.exe"),
(Join-Path (Join-Path $inDir 'Release') "$name.exe")
)
$exePath = $null
foreach ($p in $candidates) {
if (Test-Path -LiteralPath $p) {
$exePath = $p
break
}
}
if ($null -ne $exePath) {
Write-Host "Testing ${name}..."
$pinfo = New-Object System.Diagnostics.ProcessStartInfo
$pinfo.FileName = $exePath
$pinfo.Arguments = "--help"
$pinfo.RedirectStandardOutput = $true
$pinfo.RedirectStandardError = $true
$pinfo.UseShellExecute = $false
$p = New-Object System.Diagnostics.Process
$p.StartInfo = $pinfo
$p.Start() | Out-Null
$stdout = $p.StandardOutput.ReadToEnd()
$stderr = $p.StandardError.ReadToEnd()
$p.WaitForExit()
$output = $stdout + $stderr
if ([string]::IsNullOrWhiteSpace($output)) {
Write-Host "ERROR: ${name} produced no output"
$failed = $true
} else {
Write-Host $output
Write-Host "${name} ran successfully"
}
} else {
Write-Host "ERROR: ${name} not found under ${examplesBase}/${dir}/"
$failed = $true
}
}
if ($failed) { exit 1 } else { exit 0 }
& "${{ matrix.build_dir }}/bin/livekit_unit_tests.exe" `
--gtest_output=xml:${{ matrix.build_dir }}/unit-test-results.xml

- name: Upload test results
Copy link
Copy Markdown
Contributor

@stephen-derosa stephen-derosa Apr 10, 2026

Choose a reason for hiding this comment

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

<3

if: always()
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
with:
name: test-results-${{ matrix.name }}
path: ${{ matrix.build_dir }}/unit-test-results.xml
retention-days: 7

# ---------- Upload artifacts ----------
- name: Upload build artifacts
Expand Down
23 changes: 17 additions & 6 deletions bridge/include/livekit_bridge/rpc_constants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@

#include <string>

#ifdef _WIN32
#ifdef livekit_bridge_EXPORTS
#define LIVEKIT_BRIDGE_API __declspec(dllexport)
#else
#define LIVEKIT_BRIDGE_API __declspec(dllimport)
#endif
#else
#define LIVEKIT_BRIDGE_API
#endif

namespace livekit_bridge {
namespace rpc {

Expand All @@ -34,20 +44,21 @@ namespace track_control {
enum class Action { kActionMute, kActionUnmute };

/// RPC method name registered by the bridge for remote track control.
extern const char *const kMethod;
LIVEKIT_BRIDGE_API extern const char *const kMethod;

/// Payload action strings.
extern const char *const kActionMute;
extern const char *const kActionUnmute;
LIVEKIT_BRIDGE_API extern const char *const kActionMute;
LIVEKIT_BRIDGE_API extern const char *const kActionUnmute;

/// Delimiter between action and track name in the payload (e.g. "mute:cam").
extern const char kDelimiter;
LIVEKIT_BRIDGE_API extern const char kDelimiter;

/// Response payload returned on success.
extern const char *const kResponseOk;
LIVEKIT_BRIDGE_API extern const char *const kResponseOk;

/// Build a track-control RPC payload: "<action>:<track_name>".
std::string formatPayload(const char *action, const std::string &track_name);
LIVEKIT_BRIDGE_API std::string formatPayload(const char *action,
const std::string &track_name);

} // namespace track_control
} // namespace rpc
Expand Down
1 change: 1 addition & 0 deletions bridge/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ if(BRIDGE_TEST_SOURCES)
# Register tests with CTest
gtest_discover_tests(livekit_bridge_tests
WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
DISCOVERY_MODE PRE_TEST
PROPERTIES
LABELS "bridge_unit"
)
Expand Down
105 changes: 105 additions & 0 deletions src/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,91 @@ FetchContent_MakeAvailable(googletest)
enable_testing()
include(GoogleTest)

# ============================================================================
# Unit Tests
# ============================================================================

file(GLOB UNIT_TEST_SOURCES
"${CMAKE_CURRENT_SOURCE_DIR}/unit/*.cpp"
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026

Choose a reason for hiding this comment

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

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.

)

if(UNIT_TEST_SOURCES)
add_executable(livekit_unit_tests
${UNIT_TEST_SOURCES}
)

target_link_libraries(livekit_unit_tests
PRIVATE
livekit
spdlog::spdlog
GTest::gtest_main
GTest::gmock
)

target_include_directories(livekit_unit_tests
PRIVATE
${LIVEKIT_ROOT_DIR}/include
${LIVEKIT_ROOT_DIR}/src
${LIVEKIT_BINARY_DIR}/generated
${Protobuf_INCLUDE_DIRS}
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026

Choose a reason for hiding this comment

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

why protobuf is needed ?

maybe some tests are not correctly written ?

)
if(TARGET absl::base)
get_target_property(_livekit_unit_test_absl_inc absl::base INTERFACE_INCLUDE_DIRECTORIES)
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026

Choose a reason for hiding this comment

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

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 ?

if(_livekit_unit_test_absl_inc)
target_include_directories(livekit_unit_tests PRIVATE
${_livekit_unit_test_absl_inc}
)
endif()
endif()

target_compile_definitions(livekit_unit_tests
PRIVATE
LIVEKIT_TEST_ACCESS
LIVEKIT_ROOT_DIR="${LIVEKIT_ROOT_DIR}"
SPDLOG_ACTIVE_LEVEL=${_SPDLOG_ACTIVE_LEVEL}
$<$<PLATFORM_ID:Windows>:_USE_MATH_DEFINES>
)

if(WIN32)
add_custom_command(TARGET livekit_unit_tests POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy_if_different
$<TARGET_FILE:livekit>
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMAND ${CMAKE_COMMAND} -E copy_if_different
"$<TARGET_FILE_DIR:livekit>/livekit_ffi.dll"
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMENT "Copying DLLs to unit test directory"
)
elseif(APPLE)
add_custom_command(TARGET livekit_unit_tests POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy_if_different
$<TARGET_FILE:livekit>
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMAND ${CMAKE_COMMAND} -E copy_if_different
"$<TARGET_FILE_DIR:livekit>/liblivekit_ffi.dylib"
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMENT "Copying dylibs to unit test directory"
)
else()
add_custom_command(TARGET livekit_unit_tests POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy_if_different
$<TARGET_FILE:livekit>
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMAND ${CMAKE_COMMAND} -E copy_if_different
"$<TARGET_FILE_DIR:livekit>/liblivekit_ffi.so"
$<TARGET_FILE_DIR:livekit_unit_tests>
COMMENT "Copying shared libraries to unit test directory"
)
endif()

gtest_discover_tests(livekit_unit_tests
WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
DISCOVERY_MODE PRE_TEST
PROPERTIES
LABELS "unit"
)
endif()

# ============================================================================
# Integration Tests
# ============================================================================
Expand All @@ -38,10 +123,18 @@ if(INTEGRATION_TEST_SOURCES)
${INTEGRATION_TEST_SOURCES}
)

# On Windows, protobuf default-instance symbols (constinit globals) are not
# auto-exported from livekit.dll by WINDOWS_EXPORT_ALL_SYMBOLS. Link the
# proto object library directly so the test binary has its own copy.
if(WIN32 AND TARGET livekit_proto)
target_sources(livekit_integration_tests PRIVATE $<TARGET_OBJECTS:livekit_proto>)
endif()

target_link_libraries(livekit_integration_tests
PRIVATE
livekit
spdlog::spdlog
$<$<PLATFORM_ID:Windows>:${LIVEKIT_PROTOBUF_TARGET}>
GTest::gtest_main
GTest::gmock
)
Expand All @@ -67,6 +160,7 @@ if(INTEGRATION_TEST_SOURCES)
LIVEKIT_TEST_ACCESS
LIVEKIT_ROOT_DIR="${LIVEKIT_ROOT_DIR}"
SPDLOG_ACTIVE_LEVEL=${_SPDLOG_ACTIVE_LEVEL}
$<$<PLATFORM_ID:Windows>:_USE_MATH_DEFINES>
)

# Copy shared libraries to test executable directory
Expand Down Expand Up @@ -105,6 +199,7 @@ if(INTEGRATION_TEST_SOURCES)
# Register tests with CTest
gtest_discover_tests(livekit_integration_tests
WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
DISCOVERY_MODE PRE_TEST
PROPERTIES
LABELS "integration"
)
Expand Down Expand Up @@ -137,6 +232,11 @@ if(STRESS_TEST_SOURCES)
${LIVEKIT_ROOT_DIR}/src
)

target_compile_definitions(livekit_stress_tests
PRIVATE
$<$<PLATFORM_ID:Windows>:_USE_MATH_DEFINES>
)

# Copy shared libraries to test executable directory
if(WIN32)
add_custom_command(TARGET livekit_stress_tests POST_BUILD
Expand Down Expand Up @@ -173,6 +273,7 @@ if(STRESS_TEST_SOURCES)
# Register tests with CTest (longer timeout for stress tests)
gtest_discover_tests(livekit_stress_tests
WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
DISCOVERY_MODE PRE_TEST
PROPERTIES
LABELS "stress"
TIMEOUT 300
Expand All @@ -189,6 +290,10 @@ add_custom_target(run_all_tests
COMMENT "Running all tests"
)

if(TARGET livekit_unit_tests)
add_dependencies(run_all_tests livekit_unit_tests)
endif()

if(TARGET livekit_integration_tests)
add_dependencies(run_all_tests livekit_integration_tests)
endif()
Expand Down
0 src/tests/integration/test_audio_frame.cpp → src/tests/unit/test_audio_frame.cpp
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions ...egration/test_audio_processing_module.cpp → ...sts/unit/test_audio_processing_module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,7 @@ TEST_F(AudioProcessingModuleTest, AGCAttenuatesLoudSpeech) {
int sample_rate = 0;
int num_channels = 0;

// TODO: figure out what generates this welcome.wav file such that this test isn't skipped
Copy link
Copy Markdown
Contributor

@stephen-derosa stephen-derosa Apr 10, 2026

Choose a reason for hiding this comment

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

please create a ticket for this and use TODO(project-ticketid). I am guilt of not doing this as much as i should.

Copy link
Copy Markdown
Contributor Author

@alan-george-lk alan-george-lk Apr 10, 2026

Choose a reason for hiding this comment

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

This was a TODO for me before this PR, ideally I'll fix it before we merge, otherwise 100% will do

Copy link
Copy Markdown
Contributor Author

@alan-george-lk alan-george-lk Apr 10, 2026

Choose a reason for hiding this comment

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

@xianshijing-lk insights here?

Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It was something very basic, I generated it by using the the say cmd on macbook, like
say "Hello world" -o hello_world.wav

std::string wav_path = std::string(LIVEKIT_ROOT_DIR) + "/data/welcome.wav";
if (!readWavFile(wav_path, original_samples, sample_rate, num_channels)) {
GTEST_SKIP() << "Could not read " << wav_path;
Expand Down
0 src/tests/integration/test_logging.cpp → src/tests/unit/test_logging.cpp
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions ...tests/integration/test_room_callbacks.cpp → src/tests/unit/test_room_callbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ TEST_F(RoomCallbackTest, ConcurrentRegistrationDoesNotCrash) {
threads.reserve(kThreads);

for (int t = 0; t < kThreads; ++t) {
threads.emplace_back([&room, t]() {
threads.emplace_back([&room, t, kIterations]() {
for (int i = 0; i < kIterations; ++i) {
const std::string id = "participant-" + std::to_string(t);
room.setOnAudioFrameCallback(id, TrackSource::SOURCE_MICROPHONE,
Expand All @@ -228,7 +228,7 @@ TEST_F(RoomCallbackTest, ConcurrentMixedRegistrationDoesNotCrash) {
threads.reserve(kThreads);

for (int t = 0; t < kThreads; ++t) {
threads.emplace_back([&room, t]() {
threads.emplace_back([&room, t, kIterations]() {
const std::string id = "p-" + std::to_string(t);
for (int i = 0; i < kIterations; ++i) {
room.setOnAudioFrameCallback(id, TrackSource::SOURCE_MICROPHONE,
Expand Down
0 ...s/integration/test_sdk_initialization.cpp → src/tests/unit/test_sdk_initialization.cpp
View file
Open in desktop
File renamed without changes.
Loading
Loading