Copilot commented on code in PR #2304:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2304#discussion_r4562710572
##########
libminifi/test/unit/BackTraceTests.cpp:
##########
@@ -63,22 +80,31 @@ TEST_CASE("BT2", "[TPT2]") {
pool.start();
{
ready_for_checking.wait(false);
+ const auto release_worker = gsl::finally([&]() {
+ done_with_checking.test_and_set();
+ done_with_checking.notify_all();
+ });
std::vector<BackTrace> traces = pool.getTraces();
CHECK(traces.size() <= number_of_worker_threads);
REQUIRE(!traces.empty());
for (const auto &trace : traces) {
- CHECK(trace.getName().starts_with(thread_pool_name));
+ CHECK(trace.name.starts_with(thread_pool_name));
+ CHECK(std::ranges::all_of(trace.trace,
+ [](const std::string& line) { return (line.contains(" @ ") &&
line.contains(" + ")) || line == "???"; }));
}
#ifdef HAS_EXECINFO
- auto first_worker_trace = traces.front();
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("run_tasks") != trace_line.npos; }));
-#ifdef DEBUG
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("outer_function") != trace_line.npos; }));
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("inner_function") != trace_line.npos; }));
-#endif
+ if constexpr (IsDebugBuild) {
+ const auto active_trace_it = std::ranges::find_if(traces, [&](const
BackTrace& trace) {
+ return std::ranges::any_of(trace.trace, [&](const std::string& line) {
return line.contains("Worker::run"); });
Review Comment:
`std::string::contains` is a C++23 API. If this project is built with C++20
(common for MiNiFi C++), this will fail to compile. Prefer `line.find(\"...\")
!= std::string::npos` (or a small helper) to keep the tests compatible with the
project’s language standard.
##########
libminifi/test/unit/BackTraceTests.cpp:
##########
@@ -15,23 +14,39 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+#ifndef NDEBUG // TestBase.h #undef's NDEBUG
+constexpr bool IsDebugBuild = true;
+#else
+constexpr bool IsDebugBuild = false;
+#endif
-#include <utility>
+#include <algorithm>
+#include <csignal>
#include <future>
-#include <memory>
+#include <string>
+#include <utility>
+
#include "unit/TestBase.h"
#include "unit/Catch.h"
#include "utils/BackTrace.h"
#include "utils/Monitors.h"
#include "utils/ThreadPool.h"
-#include "range/v3/algorithm/any_of.hpp"
-using namespace std::literals::chrono_literals;
+using BackTrace = utils::trace_resolver::BackTrace;
-TEST_CASE("BT1", "[TPT1]") {
- const BackTrace trace = TraceResolver::getResolver().getBackTrace("BT1");
+TEST_CASE("Simple backtrace test", "[BT1]") {
+ LogTestController::getInstance().setTrace<BackTrace>();
+
+ const BackTrace back_trace = utils::trace_resolver::getBackTrace("BT1");
#ifdef HAS_EXECINFO
- CHECK(!trace.getTraces().empty());
+ const auto& lines = back_trace.trace;
+ CHECK(!lines.empty());
+ CHECK(std::ranges::all_of(lines,
+ [](const std::string& line) { return (line.contains(" @ ") &&
line.contains(" + ")) || line == "???"; }));
+ if constexpr (IsDebugBuild) {
+ CHECK(std::ranges::any_of(lines,
+ [](const std::string& line) { return line.contains("getBackTrace");
}));
Review Comment:
`std::string::contains` is a C++23 API. If this project is built with C++20
(common for MiNiFi C++), this will fail to compile. Prefer `line.find(\"...\")
!= std::string::npos` (or a small helper) to keep the tests compatible with the
project’s language standard.
##########
libminifi/test/unit/BackTraceTests.cpp:
##########
@@ -63,22 +80,31 @@ TEST_CASE("BT2", "[TPT2]") {
pool.start();
{
ready_for_checking.wait(false);
+ const auto release_worker = gsl::finally([&]() {
+ done_with_checking.test_and_set();
+ done_with_checking.notify_all();
+ });
std::vector<BackTrace> traces = pool.getTraces();
CHECK(traces.size() <= number_of_worker_threads);
REQUIRE(!traces.empty());
for (const auto &trace : traces) {
- CHECK(trace.getName().starts_with(thread_pool_name));
+ CHECK(trace.name.starts_with(thread_pool_name));
+ CHECK(std::ranges::all_of(trace.trace,
+ [](const std::string& line) { return (line.contains(" @ ") &&
line.contains(" + ")) || line == "???"; }));
}
#ifdef HAS_EXECINFO
- auto first_worker_trace = traces.front();
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("run_tasks") != trace_line.npos; }));
-#ifdef DEBUG
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("outer_function") != trace_line.npos; }));
- CHECK(ranges::any_of(first_worker_trace.getTraces(), [](const std::string&
trace_line) { return trace_line.find("inner_function") != trace_line.npos; }));
-#endif
+ if constexpr (IsDebugBuild) {
+ const auto active_trace_it = std::ranges::find_if(traces, [&](const
BackTrace& trace) {
+ return std::ranges::any_of(trace.trace, [&](const std::string& line) {
return line.contains("Worker::run"); });
+ });
+ REQUIRE(active_trace_it != traces.end());
+ const auto& lines = active_trace_it->trace;
+ CHECK(std::ranges::any_of(lines,
+ [](const std::string& line) { return
line.contains("outer_function"); }));
+ CHECK(std::ranges::any_of(lines,
+ [](const std::string& line) { return
line.contains("inner_function"); }));
Review Comment:
`std::string::contains` is a C++23 API. If this project is built with C++20
(common for MiNiFi C++), this will fail to compile. Prefer `line.find(\"...\")
!= std::string::npos` (or a small helper) to keep the tests compatible with the
project’s language standard.
##########
core-framework/include/core/ThreadedRepository.h:
##########
@@ -80,13 +80,13 @@ class ThreadedRepositoryImpl : public core::RepositoryImpl,
public virtual core:
return running_state_.load() == RunningState::Running;
}
- BackTrace getTraces() override {
+ utils::trace_resolver::BackTrace getTraces() override {
// If the thread is joinable then it is finished, we cannot interrupt and
collect its stack traces
// It can occur with an empty run() implementation like the
NoOpThreadedRepository where the stack trace is unavailable
if (getThread().joinable()) {
Review Comment:
The comment and condition appear inconsistent with `std::thread` semantics:
`joinable()` generally means the thread has an associated native thread
(running or finished-but-not-joined), which is usually the prerequisite for
signaling/inspecting it. Either the comment should be corrected, or the
predicate should be revisited to match the intended behavior (e.g., returning
empty only when there is no associated thread / no native handle available).
##########
libminifi/test/unit/CMakeLists.txt:
##########
@@ -37,4 +37,11 @@ FOREACH(testfile ${UNIT_TESTS})
ENDFOREACH()
message("-- Finished building ${UNIT_TEST_COUNT} unit test file(s)...")
+if (NOT WIN32)
+ set_target_properties(BackTraceTests PROPERTIES ENABLE_EXPORTS ON)
+ if (APPLE)
Review Comment:
`-rdynamic` is typically a Linux/GNU ld option for exporting symbols from
the main executable and is often not appropriate (or is ignored/unsupported) on
macOS. If the goal is to export symbols for symbolization, consider restricting
`-rdynamic` to Linux (e.g., via `PLATFORM_ID`) and using the appropriate macOS
linker flag (commonly `-Wl,-export_dynamic`) if needed.
##########
libminifi/test/unit/CMakeLists.txt:
##########
@@ -37,4 +37,11 @@ FOREACH(testfile ${UNIT_TESTS})
ENDFOREACH()
message("-- Finished building ${UNIT_TEST_COUNT} unit test file(s)...")
+if (NOT WIN32)
+ set_target_properties(BackTraceTests PROPERTIES ENABLE_EXPORTS ON)
+ if (APPLE)
+ target_link_options(BackTraceTests PRIVATE -rdynamic)
Review Comment:
This assumes the `BackTraceTests` target always exists; if that test target
is not created in some configurations, CMake will error at configure time.
Guard these calls with `if (TARGET BackTraceTests)` (or apply the properties at
the point where the target is defined).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
