Move tests to relevant subprojects - #615
Conversation
📝 WalkthroughWalkthroughUpdated CMake test discovery, reconfiguration behavior, output paths, and test cleanup. Added FreeRTOS-based doctest runners. Added coverage for application management and events, cryptography, services, platform utilities, FreeRTOS primitives, kernel resources, persistence, timing, and system events. Updated test synchronization, mutex cleanup, atomic state handling, and task-creation failure handling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (5)
Modules/app-module/tests/source/app_manager_test.cpp (1)
393-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two test cases depend on global state left by earlier test cases.
Line 396 requires that no app instance is Active. Line 441 has the same requirement. Both hold only when every earlier
TEST_CASEstopped its instances and doctest runs the cases in declaration order. The checks fail if a developer runs a single case with--test-case=..., or randomizes with--order-by=rand.Consider adding an explicit precondition helper that stops all remaining instances at the start of these cases, so they pass in isolation.
Modules/service-module/tests/source/service_test.cpp (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis declaration is unused and its comment is stale.
No test case in this file calls
service_instance_set_state, and no test exercises try_get/put gating. Remove the declaration, or add the test it was added for.TactilityKernel/tests/source/time_and_delay.cpp (1)
5-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoosen the upper bounds to avoid flaky timing tests.
The three tests require the measured delay to stay within one tick of the requested delay. The POSIX FreeRTOS simulator runs on a preemptible host scheduler, so a loaded CI machine can add several ticks of jitter. The lower bounds prove the delay APIs do not return early, which is the useful guarantee. Consider a larger tolerance for the upper bounds.
TactilityKernel/tests/source/preferences_test.cpp (1)
53-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire a non-null
Preferencesbefore using it.Most test cases call
preferences_openand then pass the result straight intopreferences_put_*. Ifpreferences_openreturnsnullptr, the test crashes instead of reporting a failure. Line 45 usesCHECK_NE, which records the failure but still continues into the dereference. UseREQUIRE_NEafter eachpreferences_opencall, as lines 224 and 234 already do.Proposed fix
Preferences* preferences = preferences_open(TEST_PATH); + REQUIRE_NE(preferences, nullptr); preferences_put_bool(preferences, "flag", true);TactilityKernel/tests/source/properties_file_test.cpp (1)
209-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire a non-null
PropertiesFilebefore using it.Line 210 and line 219 pass the
properties_file_openresult straight intoproperties_file_set. If the open fails, the test crashes instead of reporting a failure. AddREQUIRE_NE(file, nullptr)after theseproperties_file_opencalls, as line 188 already does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7ae0356-ff15-46f5-a6d3-620b6dcab6ea
📒 Files selected for processing (60)
Documentation/ideas.mdModules/app-module/tests/CMakeLists.txtModules/app-module/tests/source/app_event_test.cppModules/app-module/tests/source/app_manager_test.cppModules/app-module/tests/source/main.cppModules/crypt-module/tests/CMakeLists.txtModules/crypt-module/tests/source/crypt_test.cppModules/crypt-module/tests/source/hash_test.cppModules/crypt-module/tests/source/main.cppModules/service-module/tests/CMakeLists.txtModules/service-module/tests/source/main.cppModules/service-module/tests/source/service_paths_test.cppModules/service-module/tests/source/service_test.cppTactility/Tests/CMakeLists.txtTactility/Tests/Source/FileTest.cppTactility/Tests/Source/Main.cppTactility/Tests/Source/ObjectFileTest.cppTactility/Tests/Source/StringTest.cppTactility/Tests/Source/TestFile.hTactility/Tests/Source/UrlTest.cppTactilityFreeRtos/Tests/CMakeLists.txtTactilityFreeRtos/Tests/Source/DispatcherTest.cppTactilityFreeRtos/Tests/Source/DispatcherThreadTest.cppTactilityFreeRtos/Tests/Source/LockTest.cppTactilityFreeRtos/Tests/Source/Main.cppTactilityFreeRtos/Tests/Source/MessageQueueTest.cppTactilityFreeRtos/Tests/Source/MutexTest.cppTactilityFreeRtos/Tests/Source/PubSubTest.cppTactilityFreeRtos/Tests/Source/RecursiveMutexTest.cppTactilityFreeRtos/Tests/Source/SemaphoreTest.cppTactilityFreeRtos/Tests/Source/ThreadTest.cppTactilityFreeRtos/Tests/Source/TimerTest.cppTactilityKernel/tests/CMakeLists.txtTactilityKernel/tests/source/bundle_test.cppTactilityKernel/tests/source/device_get_put_test.cppTactilityKernel/tests/source/device_listener_test.cppTactilityKernel/tests/source/device_test.cppTactilityKernel/tests/source/dispatcher_test.cppTactilityKernel/tests/source/driver_integration_test.cppTactilityKernel/tests/source/driver_test.cppTactilityKernel/tests/source/file_mutex_test.cppTactilityKernel/tests/source/file_system_test.cppTactilityKernel/tests/source/main.cppTactilityKernel/tests/source/memory_test.cppTactilityKernel/tests/source/module_test.cppTactilityKernel/tests/source/mutex_test.cppTactilityKernel/tests/source/paths_test.cppTactilityKernel/tests/source/preferences_test.cppTactilityKernel/tests/source/properties_file_test.cppTactilityKernel/tests/source/recursive_mutex_test.cppTactilityKernel/tests/source/system_event_test.cppTactilityKernel/tests/source/thread_test.cppTactilityKernel/tests/source/time_and_delay.cppTactilityKernel/tests/source/timer_test.cppTests/CMakeLists.txtTests/Tactility/LICENSE-GPL-3.0.mdTests/TactilityFreeRtos/LICENSE-Apache-2.0.mdTests/TactilityKernel/LICENSE-Apache-2.0.mdTests/crypt-module/LICENSE-Apache-2.0.mdTests/service-module/LICENSE-Apache-2.0.md
💤 Files with no reviewable changes (6)
- Tests/crypt-module/LICENSE-Apache-2.0.md
- Tests/Tactility/LICENSE-GPL-3.0.md
- Tests/TactilityFreeRtos/LICENSE-Apache-2.0.md
- Documentation/ideas.md
- Tests/service-module/LICENSE-Apache-2.0.md
- Tests/TactilityKernel/LICENSE-Apache-2.0.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 16
🧹 Nitpick comments (5)
Modules/app-module/tests/source/app_manager_test.cpp (1)
393-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two test cases depend on global state left by earlier test cases.
Line 396 requires that no app instance is Active. Line 441 has the same requirement. Both hold only when every earlier
TEST_CASEstopped its instances and doctest runs the cases in declaration order. The checks fail if a developer runs a single case with--test-case=..., or randomizes with--order-by=rand.Consider adding an explicit precondition helper that stops all remaining instances at the start of these cases, so they pass in isolation.
Modules/service-module/tests/source/service_test.cpp (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis declaration is unused and its comment is stale.
No test case in this file calls
service_instance_set_state, and no test exercises try_get/put gating. Remove the declaration, or add the test it was added for.TactilityKernel/tests/source/time_and_delay.cpp (1)
5-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoosen the upper bounds to avoid flaky timing tests.
The three tests require the measured delay to stay within one tick of the requested delay. The POSIX FreeRTOS simulator runs on a preemptible host scheduler, so a loaded CI machine can add several ticks of jitter. The lower bounds prove the delay APIs do not return early, which is the useful guarantee. Consider a larger tolerance for the upper bounds.
TactilityKernel/tests/source/preferences_test.cpp (1)
53-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire a non-null
Preferencesbefore using it.Most test cases call
preferences_openand then pass the result straight intopreferences_put_*. Ifpreferences_openreturnsnullptr, the test crashes instead of reporting a failure. Line 45 usesCHECK_NE, which records the failure but still continues into the dereference. UseREQUIRE_NEafter eachpreferences_opencall, as lines 224 and 234 already do.Proposed fix
Preferences* preferences = preferences_open(TEST_PATH); + REQUIRE_NE(preferences, nullptr); preferences_put_bool(preferences, "flag", true);TactilityKernel/tests/source/properties_file_test.cpp (1)
209-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRequire a non-null
PropertiesFilebefore using it.Line 210 and line 219 pass the
properties_file_openresult straight intoproperties_file_set. If the open fails, the test crashes instead of reporting a failure. AddREQUIRE_NE(file, nullptr)after theseproperties_file_opencalls, as line 188 already does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7ae0356-ff15-46f5-a6d3-620b6dcab6ea
📒 Files selected for processing (60)
Documentation/ideas.mdModules/app-module/tests/CMakeLists.txtModules/app-module/tests/source/app_event_test.cppModules/app-module/tests/source/app_manager_test.cppModules/app-module/tests/source/main.cppModules/crypt-module/tests/CMakeLists.txtModules/crypt-module/tests/source/crypt_test.cppModules/crypt-module/tests/source/hash_test.cppModules/crypt-module/tests/source/main.cppModules/service-module/tests/CMakeLists.txtModules/service-module/tests/source/main.cppModules/service-module/tests/source/service_paths_test.cppModules/service-module/tests/source/service_test.cppTactility/Tests/CMakeLists.txtTactility/Tests/Source/FileTest.cppTactility/Tests/Source/Main.cppTactility/Tests/Source/ObjectFileTest.cppTactility/Tests/Source/StringTest.cppTactility/Tests/Source/TestFile.hTactility/Tests/Source/UrlTest.cppTactilityFreeRtos/Tests/CMakeLists.txtTactilityFreeRtos/Tests/Source/DispatcherTest.cppTactilityFreeRtos/Tests/Source/DispatcherThreadTest.cppTactilityFreeRtos/Tests/Source/LockTest.cppTactilityFreeRtos/Tests/Source/Main.cppTactilityFreeRtos/Tests/Source/MessageQueueTest.cppTactilityFreeRtos/Tests/Source/MutexTest.cppTactilityFreeRtos/Tests/Source/PubSubTest.cppTactilityFreeRtos/Tests/Source/RecursiveMutexTest.cppTactilityFreeRtos/Tests/Source/SemaphoreTest.cppTactilityFreeRtos/Tests/Source/ThreadTest.cppTactilityFreeRtos/Tests/Source/TimerTest.cppTactilityKernel/tests/CMakeLists.txtTactilityKernel/tests/source/bundle_test.cppTactilityKernel/tests/source/device_get_put_test.cppTactilityKernel/tests/source/device_listener_test.cppTactilityKernel/tests/source/device_test.cppTactilityKernel/tests/source/dispatcher_test.cppTactilityKernel/tests/source/driver_integration_test.cppTactilityKernel/tests/source/driver_test.cppTactilityKernel/tests/source/file_mutex_test.cppTactilityKernel/tests/source/file_system_test.cppTactilityKernel/tests/source/main.cppTactilityKernel/tests/source/memory_test.cppTactilityKernel/tests/source/module_test.cppTactilityKernel/tests/source/mutex_test.cppTactilityKernel/tests/source/paths_test.cppTactilityKernel/tests/source/preferences_test.cppTactilityKernel/tests/source/properties_file_test.cppTactilityKernel/tests/source/recursive_mutex_test.cppTactilityKernel/tests/source/system_event_test.cppTactilityKernel/tests/source/thread_test.cppTactilityKernel/tests/source/time_and_delay.cppTactilityKernel/tests/source/timer_test.cppTests/CMakeLists.txtTests/Tactility/LICENSE-GPL-3.0.mdTests/TactilityFreeRtos/LICENSE-Apache-2.0.mdTests/TactilityKernel/LICENSE-Apache-2.0.mdTests/crypt-module/LICENSE-Apache-2.0.mdTests/service-module/LICENSE-Apache-2.0.md
💤 Files with no reviewable changes (6)
- Tests/crypt-module/LICENSE-Apache-2.0.md
- Tests/Tactility/LICENSE-GPL-3.0.md
- Tests/TactilityFreeRtos/LICENSE-Apache-2.0.md
- Documentation/ideas.md
- Tests/service-module/LICENSE-Apache-2.0.md
- Tests/TactilityKernel/LICENSE-Apache-2.0.md
🛑 Comments failed to post (15)
Modules/crypt-module/tests/source/main.cpp (2)
46-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both runners gate the task-creation check behind
assert. When the test target is compiled withNDEBUG,assertexpands to nothing, so a failedxTaskCreateis ignored,vTaskStartSchedulerruns with no task, andmainreturns the initialdata.resultof 0. The suite then reports success without running any test.Modules/app-module/tests/source/main.cppalready uses an explicit check.
Modules/crypt-module/tests/source/main.cpp#L46-L46: replaceassert(task_result == pdPASS)withif (task_result != pdPASS) { return 1; }.Modules/service-module/tests/source/main.cpp#L46-L46: apply the same explicit check.📍 Affects 2 files
Modules/crypt-module/tests/source/main.cpp#L46-L46(this comment)Modules/service-module/tests/source/main.cpp#L46-L46
53-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check which hosts/toolchains the module tests target, and whether __assert_fail is used elsewhere. fd -t f 'CMakeLists.txt' Modules | xargs rg -n 'tests|APPLE|Darwin|CMAKE_SYSTEM_NAME' rg -n '__assert_fail|__assert_rtn|vAssertCalled' --glob '!**/build/**' fd -t f -e yml -e yaml . .github 2>/dev/null | xargs rg -n 'runs-on'Repository: TactilityProject/Tactility
Length of output: 164
🏁 Script executed:
#!/bin/bash set +e echo '--- tracked crypt-module test/build files ---' git ls-files 'Modules/crypt-module/*' | sed -n '1,160p' echo '--- candidate source files ---' fd -t f -i 'main.cpp|CMakeLists.txt' Modules | sed -n '1,160p' echo '--- symbol references ---' rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' '__assert_fail|__assert_rtn|vAssertCalled' . | sed -n '1,160p' echo '--- CI host declarations ---' if [ -d .github ]; then rg -n --glob '*.yml' --glob '*.yaml' 'runs-on|macos|ubuntu|windows|Darwin|APPLE|CMAKE_SYSTEM_NAME' .github | sed -n '1,200p' fi echo '--- source context ---' if [ -f Modules/crypt-module/tests/source/main.cpp ]; then cat -n Modules/crypt-module/tests/source/main.cpp | sed -n '1,100p' fi echo '--- nearby CMake files and relevant lines ---' while IFS= read -r f; do echo "### $f" rg -n -C 4 'crypt-module|tests|add_executable|target_link|APPLE|Darwin|CMAKE_SYSTEM_NAME' "$f" | sed -n '1,180p' done < <(fd -t f -i 'CMakeLists.txt' Modules/crypt-module Modules 2>/dev/null | sort -u | sed -n '1,80p')Repository: TactilityProject/Tactility
Length of output: 4645
🏁 Script executed:
set +e printf '%s\n' '--- source ---' cat -n Modules/crypt-module/tests/source/main.cpp 2>&1 | sed -n '1,100p' printf '%s\n' '--- nearby files ---' find Modules/crypt-module -maxdepth 4 -type f \( -name 'CMakeLists.txt' -o -name '*.cmake' \) -print 2>&1 printf '%s\n' '--- symbols ---' grep -RInE --exclude-dir=.git --exclude-dir=build '__assert_fail|__assert_rtn|vAssertCalled' Modules .github 2>&1 | sed -n '1,160p' printf '%s\n' '--- hosts ---' grep -RInE --include='*.yml' --include='*.yaml' 'runs-on|macos|ubuntu|windows' .github 2>&1 | sed -n '1,160p'Repository: TactilityProject/Tactility
Length of output: 2974
🏁 Script executed:
python3 - <<'PY' from pathlib import Path root = Path(".") source = root / "Modules/crypt-module/tests/source/main.cpp" print("SOURCE_EXISTS", source.exists()) if source.exists(): lines = source.read_text(errors="replace").splitlines() for n, line in enumerate(lines, 1): if "__assert_fail" in line or "vAssertCalled" in line or "assert" in line.lower(): lo, hi = max(1, n - 4), min(len(lines), n + 4) print(f"SOURCE_CONTEXT {lo}-{hi}") for i in range(lo, hi + 1): print(f"{i}: {lines[i-1]}") for p in sorted(root.rglob("*")): if not p.is_file() or ".git" in p.parts or "build" in p.parts: continue if p.name == "CMakeLists.txt" or p.suffix in {".yml", ".yaml"}: try: text = p.read_text(errors="replace") except Exception: continue hits = [i for i, line in enumerate(text.splitlines(), 1) if any(x in line for x in ("crypt-module", "runs-on", "APPLE", "Darwin", "CMAKE_SYSTEM_NAME", "add_executable", "target_link"))] if hits: print("FILE", p) for i in hits[:40]: lines = text.splitlines() lo, hi = max(1, i-2), min(len(lines), i+2) print(f"CONTEXT {lo}-{hi}") for j in range(lo, hi+1): print(f"{j}: {lines[j-1]}") PYRepository: TactilityProject/Tactility
Length of output: 50382
🌐 Web query:
official documentation __assert_fail glibc __assert_rtn macOS libc assert💡 Result:
Both assert_fail and assert_rtn are internal library functions used to implement the standard C assert macro [1][2][3]. They are not intended for direct use by application developers, but rather serve as the underlying handlers triggered when an assertion fails [4][3][5]. assert_fail (glibc) This function is the standard implementation for assertion failures in the GNU C Library (glibc), used on Linux and other systems following the Linux Standard Base [4][6]. When an expression passed to assert evaluates to false, the macro calls assert_fail to print a diagnostic message to stderr (including the file name, line number, and function name) and then terminates the program by calling abort [1][4][6][7]. Signature: void assert_fail(const char *assertion, const char *file, unsigned int line, const char *function); [4][6] assert_rtn (macOS/Darwin) This is the equivalent function used within the C library on Darwin-based systems, including macOS and iOS [8][9][10]. Like assert_fail, it is invoked by the assert macro when a check fails [3]. It serves the same primary purpose: printing diagnostic information and aborting the process [3][11]. The name assert_rtn is historically derived and specific to the BSD-derived C library used by Apple platforms [9][12]. Signature: void assert_rtn(const char *func, const char *file, uint line, const char *exp); [8][9][10] Key Differences The primary difference is the platform-specific implementation and the argument order [13]. While both perform identical roles, they are specific to their respective C library implementations (glibc vs. Darwin libc) [13]. Because these are internal "private" interfaces, their signatures or existence can change across library versions, and relying on them directly is generally discouraged [3][5]. Developers should always use the standard assert macro provided in <assert.h> to ensure portability [1][11].
Citations:
- 1: https://sourceware.org/glibc/manual/2.40/html_node/Consistency-Checking.html
- 2: https://codebrowser.dev/glibc/glibc/assert/assert.h.html
- 3: https://lists.llvm.org/pipermail/cfe-dev/2013-June/030077.html
- 4: https://refspecs.linuxfoundation.org/LSB_2.0.1/LSB-Core/LSB-Core/baselib---assert-fail-1.html
- 5: https://www.thecodingforums.com/threads/is-there-a-version-of-assert-that-doesnt-exit.734567/
- 6: https://fossies.org/dox/glibc-2.43/assert_8c_source.html
- 7: https://elixir.bootlin.com/glibc/glibc-2.41.9000/source/assert/assert.c
- 8: https://opendlang.org/library/core.stdc.assert_.__assert_rtn.html
- 9: http://dpldocs.info/experimental-docs/core.stdc.assert_.__assert_rtn.html
- 10: https://github.com/dlang/dmd/blob/master/druntime/src/core/stdc/assert_.d
- 11: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/assert.3.html
- 12: https://dpldocs.info/experimental-docs/core.stdc.assert_.__assert_rtn.html
- 13: https://github.com/dlang/druntime/blob/v2.098.0/src/core/stdc/assert_.d
🏁 Script executed:
pwd printf '%s\n' '--- matching tracked paths ---' git ls-files | grep -Ei '(^|/)(main\.cpp|CMakeLists\.txt)$|crypt-module|\.github/.*\.ya?ml' | sed -n '1,240p' printf '%s\n' '--- matching filesystem paths ---' find . -type f \( -name 'main.cpp' -o -name 'CMakeLists.txt' -o -name '*.yml' -o -name '*.yaml' \) -print 2>/dev/null | grep -Ei 'crypt|github|module|main|cmake' | sed -n '1,240p'Repository: TactilityProject/Tactility
Length of output: 17920
🏁 Script executed:
set -e for f in \ Modules/crypt-module/CMakeLists.txt \ Modules/crypt-module/tests/CMakeLists.txt \ CMakeLists.txt \ Platforms/platform-posix/CMakeLists.txt \ Devices/simulator/CMakeLists.txt \ .github/workflows/build-simulator.yml \ .github/actions/build-simulator/action.yml do if [ -f "$f" ]; then echo "===== $f =====" cat -n "$f" | sed -n '1,240p' fi done printf '%s\n' '===== relevant references =====' rg -n -C 5 'crypt-module|platform-posix|TactilityFreeRtos|Modules/crypt-module/tests|add_subdirectory\(.*tests|BUILD_TEST|ESP_PLATFORM|PLATFORM' \ CMakeLists.txt Modules/crypt-module Platforms/platform-posix Devices/simulator TactilityFreeRtos .github/workflows/build-simulator.yml .github/actions/build-simulator/action.yml 2>/dev/null | sed -n '1,320p'Repository: TactilityProject/Tactility
Length of output: 32833
🏁 Script executed:
set -e echo '===== Modules/crypt-module/CMakeLists.txt =====' cat -n Modules/crypt-module/CMakeLists.txt echo '===== Modules/crypt-module/tests/CMakeLists.txt =====' cat -n Modules/crypt-module/tests/CMakeLists.txt echo '===== root CMake platform/test references =====' rg -n -C 8 'add_subdirectory|ESP_PLATFORM|PLATFORM|BUILD_TEST|Tests|crypt-module|platform-posix' CMakeLists.txt | sed -n '1,260p' echo '===== simulator CMake files =====' cat -n Devices/simulator/CMakeLists.txt echo '===== platform-posix CMake =====' cat -n Platforms/platform-posix/CMakeLists.txtRepository: TactilityProject/Tactility
Length of output: 4747
🏁 Script executed:
set -e echo '===== Tests/CMakeLists.txt =====' cat -n Tests/CMakeLists.txt | sed -n '1,260p' echo '===== test workflow =====' cat -n .github/workflows/tests.yml | sed -n '1,180p' echo '===== all references to CryptModuleTests and test subdirectories =====' rg -n -C 5 'CryptModuleTests|Modules/.*/tests|add_subdirectory\(.*tests|ctest|cmake --build' \ CMakeLists.txt Tests Modules .github/workflows .github/actions \ --glob '!Libraries/**' | sed -n '1,300p'Repository: TactilityProject/Tactility
Length of output: 26815
Use a portable assertion failure handler.
The non-ESP build supports macOS, and
Tests/CMakeLists.txtincludesCryptModuleTestsinbuild-tests. macOS does not provide__assert_fail, so this test cannot compile there. Replace it withfprintf(stderr, ...)followed byabort()using<cstdio>and<cstdlib>.Tactility/Tests/Source/FileTest.cpp (1)
6-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the directories created by these tests.
These tests leave
test1/test1and/tmp/test2after execution. Use unique test paths and remove the complete directory tree after each test. This prevents state leakage between runs.Tactility/Tests/Source/Main.cpp (1)
49-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle
xTaskCreatefailure withoutassert.In an
NDEBUGbuild, Line 57 does nothing. IfxTaskCreatefails,vTaskStartScheduler()can run withouttest_taskand hang the test process. Return a failure result explicitly.Proposed fix
- assert(task_result == pdPASS); + if (task_result != pdPASS) { + return 1; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.BaseType_t task_result = xTaskCreate( test_task, "test_task", 8192, &data, 1, nullptr ); if (task_result != pdPASS) { return 1; } vTaskStartScheduler();Tactility/Tests/Source/ObjectFileTest.cpp (1)
24-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the values in the multi-record read test.
The test verifies only read success and record count. It does not verify that either returned record matches the value written. Add value assertions after both
readNext()calls.Proposed fix
CHECK_EQ(reader.hasNext(), true); CHECK_EQ(reader.readNext(&record_in), true); + CHECK_EQ(record_in.value, 0xAAAAAAAA); CHECK_EQ(reader.hasNext(), true); CHECK_EQ(reader.readNext(&record_in), true); + CHECK_EQ(record_in.value, 0xBBBBBBBB); CHECK_EQ(reader.hasNext(), false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.TestStruct record_in; ObjectFileReader reader = ObjectFileReader(TEMP_FILE, sizeof(TestStruct)); CHECK_EQ(reader.open(), true); CHECK_EQ(reader.hasNext(), true); CHECK_EQ(reader.readNext(&record_in), true); CHECK_EQ(record_in.value, 0xAAAAAAAA); CHECK_EQ(reader.hasNext(), true); CHECK_EQ(reader.readNext(&record_in), true); CHECK_EQ(record_in.value, 0xBBBBBBBB); CHECK_EQ(reader.hasNext(), false);Tactility/Tests/Source/UrlTest.cpp (1)
49-54: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename this test case to
urlEncode.This test calls
network::urlEncode, but its name isurlDecode. Correct the name so test output identifies the API under test.-TEST_CASE("urlDecode") { +TEST_CASE("urlEncode") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.TEST_CASE("urlEncode") { auto input = std::string("prefix!*'();:@&=+$,/?#[]<>%-.^_`{}|~ \\"); auto expected = std::string("prefix%21%2A%27%28%29%3B%3A%40%26%3D%2B%24%2C%2F%3F%23%5B%5D%3C%3E%25-.%5E_%60%7B%7D%7C~+%5C"); auto encoded = network::urlEncode(input); CHECK_EQ(encoded, expected); }TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp (1)
35-38: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Release every successful lock before test teardown.
The test at lines 35-38 leaves one recursive acquisition outstanding. The worker callbacks also return while they own their mutex. The next test can then run after FreeRTOS has deleted a held synchronization object.
TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp#L35-L38: Callmutex.unlock()twice before the test ends.TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp#L15-L18: Unlockmutexafterlock()succeeds and before the callback returns.TactilityFreeRtos/Tests/Source/MutexTest.cpp#L15-L18: Unlockmutexafterlock()succeeds and before the callback returns.Proposed fix
- mutex.lock(kernel::FREERTOS_MAX_TICKS); + if (mutex.lock(kernel::FREERTOS_MAX_TICKS)) { + mutex.unlock(); + } return 0;CHECK_EQ(mutex.lock(0), true); CHECK_EQ(mutex.lock(0), true); mutex.unlock(); + mutex.unlock();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.[&mutex] { if (mutex.lock(kernel::FREERTOS_MAX_TICKS)) { mutex.unlock(); } return 0; }CHECK_EQ(mutex.lock(0), true); CHECK_EQ(mutex.lock(0), true); mutex.unlock(); mutex.unlock(); }📍 Affects 2 files
TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp#L35-L38(this comment)TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp#L15-L18TactilityFreeRtos/Tests/Source/MutexTest.cpp#L15-L18TactilityFreeRtos/Tests/Source/ThreadTest.cpp (1)
25-41: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- candidate test files ---' git ls-files 'TactilityFreeRtos/Tests/Source/ThreadTest.cpp' \ 'TactilityFreeRtos/Tests/Source/DispatcherThreadTest.cpp' \ 'TactilityFreeRtos/Tests/Source/TimerTest.cpp' printf '%s\n' '--- relevant source and API references ---' rg -n -C 4 'class (Thread|Timer|DispatcherThread)|void Thread::|Thread::|Timer::|delay(Ticks|Millis)|delayTicks|delayMillis|dispatch\\(' \ TactilityFreeRtos TactilityCore 2>/dev/null | head -n 500 printf '%s\n' '--- test contents ---' for f in TactilityFreeRtos/Tests/Source/ThreadTest.cpp \ TactilityFreeRtos/Tests/Source/DispatcherThreadTest.cpp \ TactilityFreeRtos/Tests/Source/TimerTest.cpp; do if [ -f "$f" ]; then echo "### $f" cat -n "$f" fi doneRepository: TactilityProject/Tactility
Length of output: 5999
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- matching declarations and definitions ---' rg -n -S 'class[[:space:]]+(Thread|Timer|DispatcherThread)|Thread::(start|join|getTaskHandle|getState)|Timer::(start|stop)|DispatcherThread::(dispatch|stop|start)|delayTicks|delayMillis' . \ -g '!build' -g '!dist' -g '!node_modules' | head -n 800 printf '%s\n' '--- likely source files ---' git ls-files | rg -i '(thread|timer|dispatcher|freertos|kernel).*\.(h|hpp|c|cc|cpp)$' | head -n 300 printf '%s\n' '--- synchronization usage in this module ---' rg -n -S 'Semaphore|semaphore|notify|Notification|Mutex|mutex|binary|EventGroup|xQueue|portMAX_DELAY|take\\(|give\\(' TactilityFreeRtos . \ -g '*.{h,hpp,c,cc,cpp}' | head -n 800Repository: TactilityProject/Tactility
Length of output: 13816
🏁 Script executed:
#!/bin/bash set -eu for f in \ TactilityFreeRtos/Include/Tactility/Thread.h \ TactilityFreeRtos/Include/Tactility/Timer.h \ TactilityFreeRtos/Include/Tactility/DispatcherThread.h \ TactilityFreeRtos/Include/Tactility/Semaphore.h \ TactilityFreeRtos/Include/Tactility/Mutex.h \ TactilityFreeRtos/Include/Tactility/kernel/Kernel.h \ TactilityFreeRtos/Tests/Source/SemaphoreTest.cpp \ TactilityFreeRtos/Tests/Source/MutexTest.cpp; do echo "### $f" wc -l "$f" cat -n "$f" done printf '%s\n' '--- synchronization symbols ---' rg -n -S 'Semaphore|semaphore|notify|Notification|Mutex|mutex|EventGroup|xQueue|portMAX_DELAY|take|give' \ TactilityFreeRtos/Include TactilityFreeRtos/Tests/Source | head -n 800Repository: TactilityProject/Tactility
Length of output: 48302
🏁 Script executed:
#!/bin/bash set -eu for f in \ TactilityFreeRtos/Include/Tactility/Dispatcher.h \ TactilityFreeRtos/Include/Tactility/EventGroup.h \ TactilityFreeRtos/Include/Tactility/Lock.h \ TactilityFreeRtos/Include/Tactility/freertoscompat/Timers.h \ TactilityFreeRtos/Include/Tactility/freertoscompat/Task.h; do echo "### $f" cat -n "$f" done printf '%s\n' '--- timer configuration and test setup ---' rg -n -S 'configTICK_RATE_HZ|configUSE_TIMERS|configTIMER_TASK|xTimer|TimerTest|DispatcherThreadTest' \ TactilityFreeRtos Devices Platforms CMakeLists.txt .github 2>/dev/null | head -n 500 printf '%s\n' '--- analogous kernel concurrency tests ---' for f in TactilityKernel/tests/source/dispatcher_test.cpp \ TactilityKernel/tests/source/thread_test.cpp \ TactilityKernel/tests/source/timer_test.cpp; do if [ -f "$f" ]; then echo "### $f" cat -n "$f" fi doneRepository: TactilityProject/Tactility
Length of output: 27222
🌐 Web query:
FreeRTOS xTimerStop callback already executing timer daemon task behavior xTimerDelete synchronization💡 Result:
In FreeRTOS, software timer callbacks execute in the context of the timer daemon (service) task [1][2]. Because all timer API operations—including xTimerStop and xTimerDelete—send commands to the timer daemon task via a private command queue, synchronization relies on the sequential processing of this queue by the daemon task [3][2]. Behavior regarding currently executing callbacks: If you call xTimerStop or xTimerDelete while a timer's callback is already executing, the daemon task is already busy executing that callback [1]. Since the daemon task is single-threaded, it cannot process new commands (like STOP or DELETE) until the currently executing callback function returns [4][2]. Key considerations for synchronization: 1. Sequential Execution: Because the timer daemon task processes commands from its queue one by one, any call to xTimerStop or xTimerDelete made from a different task will be queued and processed only after the daemon task finishes its current task (i.e., completes the executing callback) [4][5][2]. 2. Blocking Risks: You must use a block time of 0 when calling xTimerDelete or xTimerStop from within a timer callback [4]. Blocking (using a non-zero block time) within a callback will cause the timer daemon task to stop processing the command queue, leading to a deadlock because no other timer commands can be processed and no other callbacks can execute [4][5]. 3. Race Conditions: Be aware that because timer operations are asynchronous (queued), calling xTimerStop does not guarantee that the timer's callback has not already been triggered or is currently executing [6]. If your application logic requires knowing whether the callback executed, you must implement state tracking (e.g., a flag or state machine) protected by critical sections or mutexes, rather than relying solely on the return value of timer API functions [6]. 4. Memory Safety: If you delete a timer and subsequently free its associated memory, ensure the timer is truly stopped or that your code does not attempt to reference the timer handle after the delete command has been processed by the daemon task [7][5]. If the timer daemon task has higher priority than your application tasks, and you do not use blocking calls in callbacks, the command queue is typically processed promptly [5].
Citations:
- 1: https://github.com/FreeRTOS/FreeRTOS-Kernel-Book/blob/main/ch06.md
- 2: https://forums.freertos.org/t/what-is-a-timer-daemon-task/6621
- 3: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/include/timers.h
- 4: https://forums.freertos.org/t/xtimerdelete/7742
- 5: https://forums.freertos.org/t/stop-timer-delete-timer-and-free-resource/11406
- 6: https://forums.freertos.org/t/xtimerstop-race-condition/16413
- 7: https://sourceforge.net/p/freertos/bugs/175/
🏁 Script executed:
#!/bin/bash set -eu python3 - <<'PY' from pathlib import Path import re cases = { "TactilityFreeRtos/Tests/Source/ThreadTest.cpp": [ (24, 42, "interrupted", "thread"), (44, 63, "interrupted", "thread"), (65, 86, "interrupted", "thread"), ], "TactilityFreeRtos/Tests/Source/DispatcherThreadTest.cpp": [ (17, 28, "counter", "dispatcher"), ], "TactilityFreeRtos/Tests/Source/TimerTest.cpp": [ (6, 18, "counter", "timer"), (20, 30, "counter", "timer"), (32, 44, "counter", "timer"), ], } for filename, ranges in cases.items(): text = Path(filename).read_text() lines = text.splitlines() print(f"### {filename}") for start, end, variable, kind in ranges: block = "\n".join(lines[start - 1:end]) captured = bool(re.search(rf"\[&{variable}\]", block)) writes = len(re.findall(rf"\b{re.escape(variable)}\s*(?:\+\+|=)", block)) reads = len(re.findall(rf"\b(?:CHECK(?:_EQ|_GE|_NE)?\s*\([^;\n]*\b{re.escape(variable)}\b|while\s*\([^;\n]*\b{re.escape(variable)}\b)", block)) waits = re.findall(r"\b(?:delayTicks|delayMillis|stop|join)\s*\(", block) atomic = bool(re.search(r"\bstd::atomic\b|`#include`\s*<atomic>", block)) print({ "lines": f"{start}-{end}", "kind": kind, "captured_by_callback": captured, "callback_writes": writes, "test_reads_or_stop_condition": reads, "scheduler_or_lifecycle_calls": waits, "atomic_used": atomic, }) PYRepository: TactilityProject/Tactility
Length of output: 1802
Synchronize callback state and completion.
These tests share non-atomic variables between the test task and FreeRTOS tasks.
delayTicks()anddelayMillis()do not synchronize access.Timer::stop()also does not wait for a callback that is already running.Use atomic state where appropriate and a semaphore or task notification for callback completion. Use bounded waits before assertions,
stop(), and timer destruction. Replace the exact periodic count with a synchronized lower-bound assertion.This applies to
ThreadTest.cpplines 25-41, 45-62, and 66-85;DispatcherThreadTest.cpplines 20-26; andTimerTest.cpplines 7-17, 20-29, and 33-43.📍 Affects 3 files
TactilityFreeRtos/Tests/Source/ThreadTest.cpp#L25-L41(this comment)TactilityFreeRtos/Tests/Source/ThreadTest.cpp#L45-L62TactilityFreeRtos/Tests/Source/ThreadTest.cpp#L66-L85TactilityFreeRtos/Tests/Source/DispatcherThreadTest.cpp#L20-L26TactilityFreeRtos/Tests/Source/TimerTest.cpp#L7-L17TactilityFreeRtos/Tests/Source/TimerTest.cpp#L20-L29TactilityFreeRtos/Tests/Source/TimerTest.cpp#L33-L43TactilityKernel/tests/source/main.cpp (1)
47-57: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect test-target build definitions and build-type configuration. fd -HI -t f '^(CMakeLists\.txt|.*\.cmake)$' . -x \ rg -n -C 3 'NDEBUG|CMAKE_BUILD_TYPE|target_compile_definitions|add_compile_definitions'Repository: TactilityProject/Tactility
Length of output: 16901
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate files ---' fd -HI -t f 'main\.cpp|CMakeLists\.txt|.*\.cmake$' TactilityKernel . 2>/dev/null | head -200 printf '%s\n' '--- references to TactilityKernel test target/source ---' rg -n -C 4 'TactilityKernel|tests/source/main\.cpp|xTaskCreate|vTaskStartScheduler' \ --glob '!Libraries/**' --glob '!**/build/**' --glob '!**/dist/**' . printf '%s\n' '--- direct NDEBUG and assertion configuration ---' rg -n -C 3 '\bNDEBUG\b|CMAKE_CXX_FLAGS(_[A-Z]+)?|target_compile_options|add_compile_options|BUILD_TYPE' \ TactilityKernel Buildscripts CMakeLists.txt Devices 2>/dev/null || trueRepository: TactilityProject/Tactility
Length of output: 50383
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- TactilityKernel test files ---' git ls-files 'TactilityKernel/Tests/*' 'TactilityKernel/tests/*' 'Tactility/Tests/Source/Main.cpp' 'Tactility/Tests/CMakeLists.txt' 'Tests/CMakeLists.txt' printf '%s\n' '--- target build definitions ---' for f in TactilityKernel/Tests/CMakeLists.txt TactilityKernel/tests/CMakeLists.txt Tactility/Tests/Source/Main.cpp Tactility/Tests/CMakeLists.txt Tests/CMakeLists.txt CMakeLists.txt; do if [ -f "$f" ]; then echo "### $f" cat -n "$f" fi done printf '%s\n' '--- build configuration definitions ---' rg -n -C 4 'CMAKE_BUILD_TYPE|CMAKE_CXX_FLAGS|NDEBUG|Release|Debug|Coverage|add_compile_options|target_compile_options' \ CMakeLists.txt Tests TactilityKernel Tactility TactilityFreeRtos Devices/simulator Buildscripts 2>/dev/null || true printf '%s\n' '--- POSIX FreeRTOS implementation references ---' rg -n -C 5 'BaseType_t xTaskCreate|xTaskCreate\(|vTaskStartScheduler|taskYIELD|configMAX_PRIORITIES|configTOTAL_HEAP_SIZE|pvPortMalloc' \ Libraries/FreeRTOS-Kernel TactilityFreeRtos Devices/simulator Platforms 2>/dev/null | head -500Repository: TactilityProject/Tactility
Length of output: 50382
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- task and scheduler implementation files ---' rg -l 'BaseType_t\s+xTaskCreate|xTaskCreate\s*\(|void\s+vTaskStartScheduler|vTaskStartScheduler\s*\(' \ Libraries/FreeRTOS-Kernel --glob '*.[ch]' --glob '*.[ch]pp' | sort printf '%s\n' '--- exact implementation snippets ---' rg -n -C 18 'BaseType_t\s+xTaskCreate|void\s+vTaskStartScheduler|xTaskGenericCreate|prvInitialiseNewTask' \ Libraries/FreeRTOS-Kernel/portable Libraries/FreeRTOS-Kernel/tasks.c \ --glob '*.[ch]' --glob '*.[ch]pp' | head -600 printf '%s\n' '--- test build invocations and configurations ---' rg -n -C 4 'cmake|CMAKE_BUILD_TYPE|build-tests|TactilityKernelTests|ctest|Release|Debug|NDEBUG' \ .github Buildscripts Documentation README.md Tests CMakeLists.txt \ --glob '!**/doctest.h' --glob '!**/third_party/**' 2>/dev/null | head -800Repository: TactilityProject/Tactility
Length of output: 41152
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- scheduler implementation ---' sed -n '1954,2045p' Libraries/FreeRTOS-Kernel/tasks.c printf '%s\n' '--- xTaskCreate allocation implementation ---' sed -n '719,813p' Libraries/FreeRTOS-Kernel/tasks.c printf '%s\n' '--- POSIX/portable files and heap configuration ---' fd -HI -t f . Libraries/FreeRTOS-Kernel | rg -i 'posix|heap|port|CMakeLists|FreeRTOSConfig' rg -n -C 5 'FREERTOS_PORT|GCC_POSIX|heap_4|pvPortMalloc|configTOTAL_HEAP_SIZE|vPortGetHeapStats' \ Libraries/FreeRTOS-Kernel Devices/simulator CMakeLists.txt TactilityKernel/tests .github \ --glob '!**/doctest.h' 2>/dev/null | head -500 printf '%s\n' '--- focused source/config verifier ---' python3 - <<'PY' from pathlib import Path paths = [ Path("TactilityKernel/tests/source/main.cpp"), Path("TactilityKernel/tests/CMakeLists.txt"), Path("Tests/CMakeLists.txt"), Path("CMakeLists.txt"), Path(".github/workflows/tests.yml"), Path("Devices/simulator/Source/FreeRTOSConfig.h"), Path("Libraries/FreeRTOS-Kernel/tasks.c"), ] for path in paths: text = path.read_text(errors="replace") print(f"{path}: NDEBUG occurrences={text.count('NDEBUG')}") main = Path("TactilityKernel/tests/source/main.cpp").read_text() assert "assert(task_result == pdPASS);" in main assert "vTaskStartScheduler();" in main assert main.index("assert(task_result == pdPASS);") < main.index("vTaskStartScheduler();") tasks = Path("Libraries/FreeRTOS-Kernel/tasks.c").read_text() needle = "xReturn = errCOULD_NOT_ALLOCATE_REQUIRED_MEMORY;" print(f"tasks.c: allocation-failure return occurrences={tasks.count(needle)}") print(f"tasks.c: scheduler creates idle task={ 'xTaskCreateStatic' in tasks or 'xTaskCreate' in tasks }") PYRepository: TactilityProject/Tactility
Length of output: 50383
Handle task creation failure without
assert.If a build defines
NDEBUG,assert(task_result == pdPASS)does not check the result. IfxTaskCreatefails, return an error before callingvTaskStartScheduler; otherwise,TactilityKernelTestsstarts with only the idle task and can hang.TactilityKernel/tests/source/module_test.cpp (1)
140-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the stale comment and stop the module before destructing it.
Line 141 states "Still fails as symbols are null", but line 142 asserts the resolution succeeds. The comment contradicts the assertion.
The cleanup also removes and destructs the module while it is still started. The "module_ensure_started" test at lines 190-192 calls
module_stopfirst. Align both cleanup paths.Proposed fix
REQUIRE_EQ(module_start(&module), ERROR_NONE); - // Still fails as symbols are null + // Now resolvable: the module is added to the ledger and started. CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true); // Cleanup + CHECK_EQ(module_stop(&module), ERROR_NONE); CHECK_EQ(module_remove(&module), ERROR_NONE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.REQUIRE_EQ(module_start(&module), ERROR_NONE); // Now resolvable: the module is added to the ledger and started. CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true); // Cleanup CHECK_EQ(module_stop(&module), ERROR_NONE); CHECK_EQ(module_remove(&module), ERROR_NONE); CHECK_EQ(module_destruct(&module), ERROR_NONE);TactilityKernel/tests/source/mutex_test.cpp (1)
44-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The cross-task blocking tests destruct a mutex that the spawned task still holds. In both files the task locks the mutex, increments the counter, and calls
vTaskDelete(nullptr)without unlocking. The test task then waits a fixed 2 ticks and destructs a stack-allocated mutex that the task still references.
TactilityKernel/tests/source/mutex_test.cpp#L44-L65: callmutex_unlock(mutex_ptr)in the task body beforevTaskDelete(nullptr).TactilityKernel/tests/source/recursive_mutex_test.cpp#L64-L85: callrecursive_mutex_unlock(mutex_ptr)in the task body beforevTaskDelete(nullptr).📍 Affects 2 files
TactilityKernel/tests/source/mutex_test.cpp#L44-L65(this comment)TactilityKernel/tests/source/recursive_mutex_test.cpp#L64-L85TactilityKernel/tests/source/preferences_test.cpp (1)
191-191: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct this test case name.
The name says "on an already-closed value", but the test writes to a reopened
Preferencesinstance. Rename it to describe the reopen-then-update behavior it verifies.TactilityKernel/tests/source/properties_file_test.cpp (1)
30-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Duplicated
write_rawhelper skips thefopennull check in both test files. Both files define the same helper, and both pass an uncheckedFILE*tostd::fputsandstd::fclose, so an open failure crashes the test process instead of reporting a failure.
TactilityKernel/tests/source/properties_file_test.cpp#L30-L34: addREQUIRE(file != nullptr);afterstd::fopen.TactilityKernel/tests/source/preferences_test.cpp#L34-L38: add the sameREQUIRE(file != nullptr);afterstd::fopen.
ScratchFile,file_exists, andwrite_raware duplicated across both files. Consider moving them into a shared test helper header underTactilityKernel/tests/source/.🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 32-32: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
📍 Affects 2 files
TactilityKernel/tests/source/properties_file_test.cpp#L30-L34(this comment)TactilityKernel/tests/source/preferences_test.cpp#L34-L38Source: Linters/SAST tools
TactilityKernel/tests/source/system_event_test.cpp (1)
253-253: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a larger join timeout for the emitter threads.
Line 253 joins the emitter thread with a 2-tick timeout. Lines 329 and 360 do the same. The awaiting task resumes as soon as the emitter calls
system_event_emit, so the emitter may still be inside its exit path whenthread_joinstarts. Lines 402 and 435 already usepdMS_TO_TickS(2000)-scale timeouts. Align these three joins with that value to remove the flakiness.Proposed fix
- CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE); + CHECK_EQ(thread_join(thread, pdMS_TO_TICKS(2000), pdMS_TO_TICKS(1)), ERROR_NONE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.CHECK_EQ(thread_join(thread, pdMS_TO_TICKS(2000), pdMS_TO_TICKS(1)), ERROR_NONE);TactilityKernel/tests/source/thread_test.cpp (1)
100-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does doctest report an error for duplicated TEST_CASE names?💡 Result:
No, the doctest C++ testing framework does not report an error for duplicated TEST_CASE names [1][2][3]. The framework allows test cases to share the same name without restriction [2][3]. When multiple test cases have the same name, doctest will execute each of them as distinct tests [1]. Test cases are uniquely identified by the framework based on their registration (internal metadata including file and line number), not by the string literal used as the name [4][3]. Note that this behavior is specific to the C++ doctest library [2]. It is distinct from the Python doctest module (found in the standard library), which serves a different purpose and operates under different rules [5][6].
Citations:
- 1: doctest/doctest#802
- 2: https://github.com/doctest/doctest/blob/master/doc/markdown/testcases.md
- 3: doctest/doctest#40
- 4: doctest/doctest#45
- 5: https://docs.python.org/3/library/doctest.html
- 6: https://docs.python.org/release/3.11.5/library/doctest.html
🏁 Script executed:
#!/bin/bash set -eu file='TactilityKernel/tests/source/thread_test.cpp' printf '%s\n' '--- target tests ---' sed -n '1,125p' "$file" printf '%s\n' '--- doctest references and version context ---' rg -n 'doctest|TEST_CASE|thread return code|thread id should only' \ TactilityKernel/tests "$file" CMakeLists.txt '**/CMakeLists.txt' 2>/dev/null | head -200Repository: TactilityProject/Tactility
Length of output: 22052
Rename the duplicate test case.
The test at line 100 verifies the return code, not the thread ID. Use a distinct name so
--test-casecan select it independently.
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f3f89ac-7d15-45ad-84ce-56d4816b61c6
⛔ Files ignored due to path filters (1)
.claude/rules.zipis excluded by!**/*.zip
📒 Files selected for processing (27)
.claude/rules/tests.mdModules/app-module/tests/CMakeLists.txtModules/crypt-module/tests/CMakeLists.txtModules/crypt-module/tests/source/main.cppModules/service-module/tests/CMakeLists.txtModules/service-module/tests/source/main.cppTactility/Source/file/File.cppTactility/Tests/CMakeLists.txtTactility/Tests/Source/FileTest.cppTactility/Tests/Source/Main.cppTactility/Tests/Source/ObjectFileTest.cppTactility/Tests/Source/UrlTest.cppTactilityFreeRtos/Tests/CMakeLists.txtTactilityFreeRtos/Tests/Source/DispatcherThreadTest.cppTactilityFreeRtos/Tests/Source/MutexTest.cppTactilityFreeRtos/Tests/Source/RecursiveMutexTest.cppTactilityFreeRtos/Tests/Source/ThreadTest.cppTactilityFreeRtos/Tests/Source/TimerTest.cppTactilityKernel/tests/CMakeLists.txtTactilityKernel/tests/source/main.cppTactilityKernel/tests/source/module_test.cppTactilityKernel/tests/source/mutex_test.cppTactilityKernel/tests/source/preferences_test.cppTactilityKernel/tests/source/properties_file_test.cppTactilityKernel/tests/source/recursive_mutex_test.cppTactilityKernel/tests/source/system_event_test.cppTactilityKernel/tests/source/thread_test.cpp
🚧 Files skipped from review as they are similar to previous changes (22)
- TactilityFreeRtos/Tests/Source/DispatcherThreadTest.cpp
- TactilityFreeRtos/Tests/Source/MutexTest.cpp
- Modules/crypt-module/tests/CMakeLists.txt
- TactilityKernel/tests/source/mutex_test.cpp
- TactilityFreeRtos/Tests/Source/RecursiveMutexTest.cpp
- Modules/app-module/tests/CMakeLists.txt
- Tactility/Tests/Source/FileTest.cpp
- Tactility/Tests/CMakeLists.txt
- TactilityKernel/tests/source/main.cpp
- Modules/service-module/tests/CMakeLists.txt
- TactilityKernel/tests/source/recursive_mutex_test.cpp
- TactilityFreeRtos/Tests/CMakeLists.txt
- TactilityKernel/tests/CMakeLists.txt
- TactilityKernel/tests/source/module_test.cpp
- Tactility/Tests/Source/UrlTest.cpp
- Tactility/Tests/Source/ObjectFileTest.cpp
- TactilityKernel/tests/source/properties_file_test.cpp
- TactilityKernel/tests/source/thread_test.cpp
- TactilityKernel/tests/source/preferences_test.cpp
- Modules/service-module/tests/source/main.cpp
- TactilityFreeRtos/Tests/Source/ThreadTest.cpp
- Tactility/Tests/Source/Main.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03f76d92-f4d2-41b4-942e-0baf86ba169e
📒 Files selected for processing (3)
Modules/crypt-module/tests/source/main.cppTactility/Source/file/File.cppTactilityFreeRtos/Tests/Source/TimerTest.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Modules/crypt-module/tests/source/main.cpp
- Tactility/Source/file/File.cpp
| void waitForTimerServiceIdle(Timer& timer) { | ||
| Semaphore done(1, 0); | ||
| auto markDone = [](void* context, uint32_t) { | ||
| static_cast<Semaphore*>(context)->release(); | ||
| }; | ||
| REQUIRE(timer.setPendingCallback(markDone, &done, 0, pdMS_TO_TICKS(2000))); | ||
| REQUIRE(done.acquire(pdMS_TO_TICKS(2000))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'REQUIRE|DOCTEST_CONFIG_NO_EXCEPTIONS|TestFailureException' \
TactilityFreeRtos/Tests TactilityFreeRtos Libraries 2>/dev/null || true
rg -n -C 6 \
'waitForTimerServiceIdle|setPendingCallback|xTimerPendFunctionCall' \
TactilityFreeRtos/Tests/Source/TimerTest.cpp \
TactilityFreeRtos/Include/Tactility/Timer.hRepository: TactilityProject/Tactility
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test helper and surrounding tests ---'
sed -n '1,240p' TactilityFreeRtos/Tests/Source/TimerTest.cpp
printf '%s\n' '--- timer callback API and implementation references ---'
rg -n -C 8 'setPendingCallback|xTimerPendFunctionCall|TimerCallback|Semaphore' \
TactilityFreeRtos/Include TactilityFreeRtos/Source TactilityFreeRtos/Tests/Source \
-g '*.h' -g '*.hpp' -g '*.cpp' -g '*.c' 2>/dev/null | head -n 1200
printf '%s\n' '--- doctest configuration and assertion behavior references ---'
rg -n -C 8 'DOCTEST_CONFIG_NO_EXCEPTIONS|TestFailureException|REQUIRE\(' \
TactilityFreeRtos/Tests TactilityFreeRtos -g '*.h' -g '*.hpp' -g '*.cpp' -g '*.c' 2>/dev/null | head -n 1200Repository: TactilityProject/Tactility
Length of output: 35940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- doctest configuration ---'
git ls-files | rg -i '(^|/)(doctest|test|main|CMakeLists|meson|platformio|idf_component).*' | head -n 500
rg -n -C 5 'DOCTEST_CONFIG_NO_EXCEPTIONS|DOCTEST_CONFIG_IMPLEMENT|TestFailureException|TEST_CASE|REQUIRE\(' \
--glob '!Libraries/SDL/**' --glob '!Libraries/lvgl/**' --glob '!Libraries/mbedtls/**' \
. 2>/dev/null | head -n 1600
printf '%s\n' '--- exact Timer API and callback declarations ---'
sed -n '1,155p' TactilityFreeRtos/Include/Tactility/Timer.h
printf '%s\n' '--- timer service and pending-call implementation references ---'
rg -n -C 12 'xTimerPendFunctionCall|timer service|TimerPendedFunction|pend.*function|Pended' \
TactilityFreeRtos Libraries -g '*.c' -g '*.cpp' -g '*.h' -g '*.hpp' 2>/dev/null | head -n 1600Repository: TactilityProject/Tactility
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- doctest files and configuration ---'
git ls-files | rg -i 'doctest|test[_-]?main|main[_-]?test|tests?/.*main' | head -n 300
rg -n 'DOCTEST_CONFIG_NO_EXCEPTIONS|TestFailureException' \
TactilityFreeRtos TactilityKernel cmake CMakeLists.txt 2>/dev/null | head -n 300 || true
printf '%s\n' '--- Timer.h relevant declarations ---'
nl -ba TactilityFreeRtos/Include/Tactility/Timer.h | sed -n '20,145p'
printf '%s\n' '--- pending callback implementations in project files ---'
rg -n 'xTimerPendFunctionCall|setPendingCallback' \
TactilityFreeRtos TactilityKernel -g '*.c' -g '*.cpp' -g '*.h' -g '*.hpp' 2>/dev/null | head -n 300
printf '%s\n' '--- test target definitions ---'
rg -n -C 6 'TimerTest.cpp|doctest|DOCTEST' \
TactilityFreeRtos -g 'CMakeLists.txt' -g '*.cmake' -g '*.cpp' -g '*.h' 2>/dev/null | head -n 600Repository: TactilityProject/Tactility
Length of output: 635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- doctest configuration ---'
rg -n -C 8 'DOCTEST_CONFIG_NO_EXCEPTIONS|TestFailureException|DOCTEST_CONFIG_IMPLEMENT' \
TactilityFreeRtos/Tests/Source/Main.cpp \
TactilityFreeRtos/Tests \
Tests/Doctest/Include/doctest.h 2>/dev/null | head -n 500 || true
printf '%s\n' '--- Timer.h relevant declarations ---'
cat -n TactilityFreeRtos/Include/Tactility/Timer.h | sed -n '20,145p'
printf '%s\n' '--- pending callback implementations in project files ---'
rg -n -C 10 'xTimerPendFunctionCall|setPendingCallback' \
TactilityFreeRtos TactilityKernel -g '*.c' -g '*.cpp' -g '*.h' -g '*.hpp' 2>/dev/null | head -n 500
printf '%s\n' '--- test target definitions ---'
rg -n -C 6 'TimerTest.cpp|doctest|DOCTEST' \
TactilityFreeRtos -g 'CMakeLists.txt' -g '*.cmake' -g '*.cpp' -g '*.h' 2>/dev/null | head -n 500Repository: TactilityProject/Tactility
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
timer_test = Path("TactilityFreeRtos/Tests/Source/TimerTest.cpp").read_text()
timer_h = Path("TactilityFreeRtos/Include/Tactility/Timer.h").read_text()
semaphore_h = Path("TactilityFreeRtos/Include/Tactility/Semaphore.h").read_text()
doctest_h = Path("Tests/Doctest/Include/doctest.h").read_text()
checks = {
"automatic completion context": re.search(
r"void waitForTimerServiceIdle\(Timer& timer\)\s*\{.*?Semaphore done\(1,\s*0\);",
timer_test, re.S,
),
"stack context passed to asynchronous callback": re.search(
r"setPendingCallback\(markDone,\s*&done,", timer_test,
),
"callback dereferences context": re.search(
r"static_cast<Semaphore\*>(context\)->release\(\);", timer_test,
),
"pending callback uses timer service queue": re.search(
r"xTimerPendFunctionCall\(newCallback,\s*callbackContext,\s*callbackArg,\s*timeout\)",
timer_h,
),
"Semaphore owns a deletable handle": re.search(
r"std::unique_ptr<.*SemaphoreHandleDeleter>\s+handle;", semaphore_h,
),
"Semaphore destructor is present": re.search(
r"~Semaphore\(\)\s+override\s*\{", semaphore_h,
),
"doctest throws on failed assertion when exceptions are enabled": re.search(
r"void throwException\(\)\s*\{\s*g_cs->shouldLogCurrentException\s*=\s*false;\s*throw TestFailureException\(\);",
doctest_h, re.S,
),
"doctest catches the failure outside the test body": re.search(
r"catch\(const TestFailureException&\)\s*\{",
doctest_h,
),
}
for name, match in checks.items():
print(f"{name}: {'present' if match else 'missing'}")
if not all(checks.values()):
raise SystemExit("source invariant check failed")
print("Conclusion: a failed second REQUIRE can unwind after enqueueing the callback, while the callback still holds &done.")
PYRepository: TactilityProject/Tactility
Length of output: 988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
timer_test = Path("TactilityFreeRtos/Tests/Source/TimerTest.cpp").read_text()
timer_h = Path("TactilityFreeRtos/Include/Tactility/Timer.h").read_text()
semaphore_h = Path("TactilityFreeRtos/Include/Tactility/Semaphore.h").read_text()
doctest_h = Path("Tests/Doctest/Include/doctest.h").read_text()
checks = {
"automatic completion context":
"Semaphore done(1, 0);" in timer_test,
"stack context passed to asynchronous callback":
"setPendingCallback(markDone, &done," in timer_test,
"callback dereferences context":
"static_cast<Semaphore*>(context)->release();" in timer_test,
"pending callback uses timer service queue":
"xTimerPendFunctionCall(newCallback, callbackContext, callbackArg, timeout)" in timer_h,
"Semaphore owns a deletable handle":
"SemaphoreHandleDeleter> handle;" in semaphore_h,
"Semaphore destructor is present":
"~Semaphore() override" in semaphore_h,
"doctest throws on failed assertion":
"throw TestFailureException();" in doctest_h,
"doctest catches the failure outside the test body":
"catch(const TestFailureException&)" in doctest_h,
}
for name, present in checks.items():
print(f"{name}: {'present' if present else 'missing'}")
if not all(checks.values()):
raise SystemExit("source invariant check failed")
print("Conclusion: the helper passes a stack Semaphore to a timer-service callback, and a failed REQUIRE can unwind before that callback finishes.")
PYRepository: TactilityProject/Tactility
Length of output: 674
Keep the completion context alive until markDone returns.
If done.acquire(...) times out, REQUIRE unwinds the helper while the timer-service callback can still execute. markDone then dereferences the destroyed stack Semaphore, whose owned FreeRTOS handle has also been deleted. Use a completion context that remains valid through the callback, including the timeout path.
Source: MCP tools
| const auto first_run_count = counter.load(); | ||
|
|
||
| CHECK_EQ(timer->start(), true); | ||
| kernel::delayTicks(10); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the first periodic run produced a callback.
first_run_count is only used as a baseline. If the first run produces zero callbacks and the second run produces one, CHECK_GT(counter.load(), first_run_count) passes. The test then does not prove that the timer worked before restart.
Add a positive assertion before the second start():
🧪 Proposed assertion
const auto first_run_count = counter.load();
+ CHECK_GT(first_run_count, 0);
CHECK_EQ(timer->start(), true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const auto first_run_count = counter.load(); | |
| CHECK_EQ(timer->start(), true); | |
| kernel::delayTicks(10); | |
| const auto first_run_count = counter.load(); | |
| CHECK_GT(first_run_count, 0); | |
| CHECK_EQ(timer->start(), true); | |
| kernel::delayTicks(10); |
| CHECK_EQ(timer->start(), true); | ||
| kernel::delayTicks(10); | ||
| CHECK_EQ(timer->stop(), true); | ||
| waitForTimerServiceIdle(*timer); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'restarting TimerType::Once|timer->stop\(\)|timer->start\(\)|waitForTimerServiceIdle' \
TactilityFreeRtos/Tests/Source/TimerTest.cpp \
TactilityFreeRtos/Include/Tactility/Timer.hRepository: TactilityProject/Tactility
Length of output: 4943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- TimerTest.cpp ---'
cat -n TactilityFreeRtos/Tests/Source/TimerTest.cpp | sed -n '1,90p'
printf '%s\n' '--- Timer.h ---'
cat -n TactilityFreeRtos/Include/Tactility/Timer.h | sed -n '1,220p'
printf '%s\n' '--- timer implementation and test references ---'
rg -n -C 5 'Timer::(start|stop)|xTimer(Start|Stop)|timer service|waitForTimerServiceIdle' \
TactilityFreeRtos TactilityFreeRtos/TestsRepository: TactilityProject/Tactility
Length of output: 19063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- timer API implementations and compatibility layers ---'
rg -n -C 6 \
'xTimerStop|xTimerStart|xTimerPendFunctionCall|TimerService|timer command|tmrCOMMAND_STOP|tmrCOMMAND_START' \
. -g '!build' -g '!dist' -g '!node_modules' | sed -n '1,260p'
printf '%s\n' '--- relevant repository files ---'
git ls-files | rg '(^|/)(Timer|Timers|FreeRTOS|freertos|TimerTest)' | sed -n '1,160p'
printf '%s\n' '--- deterministic queue-order model ---'
python3 - <<'PY'
from collections import deque
def process(commands, expiry_pending):
queue = deque(commands)
callbacks = 0
active = True
while queue:
command = queue.popleft()
if command == "expiry":
if active:
callbacks += 1
active = False
elif command == "stop":
active = False
elif command == "start":
active = True
elif command == "marker":
return callbacks, active, list(queue)
return callbacks, active, []
for first_expiry in (False, True):
without_wait = ["stop", "start", "expiry" if first_expiry else "marker", "stop", "marker"]
with_wait = ["stop", "marker", "start", "expiry" if first_expiry else "marker", "stop", "marker"]
print({
"expiry_already_queued": first_expiry,
"without_wait": process(without_wait, first_expiry),
"with_wait": process(with_wait, first_expiry),
})
PYRepository: TactilityProject/Tactility
Length of output: 24876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- FreeRTOS timer command implementation ---'
fd -i 'timers.c' Libraries TactilityFreeRtos TactilityKernel Platforms Devices
fd -i '*timer*.c' Libraries/FreeRTOS-Kernel | sed -n '1,80p'
printf '%s\n' '--- timer command and callback processing ---'
rg -n -C 8 \
'xTimerGenericCommand|prvProcessReceivedCommands|prvProcessExpiredTimer|tmrCOMMAND_EXECUTE_CALLBACK|xTimerPendFunctionCall' \
Libraries/FreeRTOS-Kernel -g '*.c' -g '*.h' | sed -n '1,360p'Repository: TactilityProject/Tactility
Length of output: 534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- timer command and callback processing ---'
rg -n -C 10 \
'xTimerGenericCommand|prvProcessReceivedCommands|prvProcessExpiredTimer|tmrCOMMAND_EXECUTE_CALLBACK|xTimerPendFunctionCall' \
Libraries/FreeRTOS-Kernel/timers.c Libraries/FreeRTOS-Kernel/include/timers.h | sed -n '1,420p'Repository: TactilityProject/Tactility
Length of output: 34799
Synchronize the first one-shot run before restarting.
Timer::stop() only queues a command and does not wait for timer-service processing. Add waitForTimerServiceIdle(*timer) after the first stop() and before the second start(). Otherwise, the first callback can be suppressed, making the exact-two assertion scheduling-dependent.
Source: MCP tools
1 participant
Uh oh!
There was an error while loading. Please reload this page.