From c1a6854efbad489fdf36cfd55aeaba7163e505da Mon Sep 17 00:00:00 2001 From: Viktor Kopp Date: Mon, 9 Sep 2024 17:04:12 +0200 Subject: [PATCH] Testable search (#505) * Make tempPayload a local var * establish tools lib and tests infra * rm redundant SearchDialog::is_TimeStampRangeValid state var If timestamp range search is selected the loop will only continue if range is valid. It does not make sense to check in functions below if range is valid since otherwise that code cannot be even reached * implement matching for ctx and app Ids * use ctxid and appid match functionality in app * TimestampRange * use new timestamp filtering * text search using dltmessagematcher * fix build move matcher to qdlt lib enable c++17 in qmake config The new code uses variant and optional added in c++17 standard while qmake config was setup restricted to c++11 enable tests only if gtest package found Set c++17 standard in cmake export class for windows linker Signed-off-by: Viktor Kopp --- BuildDltViewer.pro | 2 +- CMakeLists.txt | 2 +- qdlt/CMakeLists.txt | 12 +- qdlt/dltmessagematcher.cpp | 66 ++++++++ qdlt/dltmessagematcher.h | 72 ++++++++ qdlt/qdlt.pro | 6 +- qdlt/tests/CMakeLists.txt | 10 ++ qdlt/tests/test_dltmessagematcher.cpp | 122 ++++++++++++++ src/searchdialog.cpp | 231 +++++--------------------- src/searchdialog.h | 6 +- src/src.pro | 3 +- 11 files changed, 331 insertions(+), 201 deletions(-) create mode 100644 qdlt/dltmessagematcher.cpp create mode 100644 qdlt/dltmessagematcher.h create mode 100644 qdlt/tests/CMakeLists.txt create mode 100644 qdlt/tests/test_dltmessagematcher.cpp diff --git a/BuildDltViewer.pro b/BuildDltViewer.pro index 8c25fad4..fb038814 100644 --- a/BuildDltViewer.pro +++ b/BuildDltViewer.pro @@ -2,7 +2,7 @@ TEMPLATE = subdirs CONFIG += ordered SUBDIRS += qdlt src plugin commander -CONFIG += c++11 +CONFIG += c++1z ICON = Project.icns QMAKE_INFO_PLIST = Info.plist diff --git a/CMakeLists.txt b/CMakeLists.txt index 00cfc9f5..f285aa2a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,7 +31,7 @@ if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") set(LINUX TRUE) endif() -set(CMAKE_CXX_STANDARD 11) +set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) diff --git a/qdlt/CMakeLists.txt b/qdlt/CMakeLists.txt index 288b673b..4ee1e017 100644 --- a/qdlt/CMakeLists.txt +++ b/qdlt/CMakeLists.txt @@ -39,7 +39,9 @@ add_library(qdlt SHARED qdltsettingsmanager.cpp qdltexporter.cpp qdltimporter.cpp - fieldnames.cpp) + fieldnames.cpp + dltmessagematcher.cpp + dltmessagematcher.h) target_compile_definitions(qdlt PRIVATE BYTE_ORDER=LITTLE_ENDIAN @@ -116,3 +118,11 @@ foreach(SDK_EXAMPLE IN ITEMS ${SDK_EXAMPLES}) DESTINATION "${DLT_ADDITIONAL_FILES_INSTALLATION_PATH}/src" COMPONENT qdlt_sdk) endforeach() + + +find_package(GTest) +# configure unit tests only if gtest found on the system +if (GTest_FOUND) + enable_testing() + add_subdirectory(tests) +endif() diff --git a/qdlt/dltmessagematcher.cpp b/qdlt/dltmessagematcher.cpp new file mode 100644 index 00000000..2426b001 --- /dev/null +++ b/qdlt/dltmessagematcher.cpp @@ -0,0 +1,66 @@ +#include "dltmessagematcher.h" + +#include + +DltMessageMatcher::DltMessageMatcher() {} + +bool DltMessageMatcher::match(const QDltMsg &msg, const Pattern& pattern) const +{ + if (!matchAppId(msg.getApid()) || !matchCtxId(msg.getCtid())) + return false; + + if (!matchTimestampRange(msg.getTimestamp())) { + return false; + } + + bool matchFound = false; + if (m_headerSearchEnabled) { + auto header = msg.toStringHeader(); + if (m_messageIdFormat) + header += ' ' + QString::asprintf(m_messageIdFormat->toUtf8(), msg.getMessageId()); + if (std::holds_alternative(pattern)) { + matchFound = header.contains(std::get(pattern)); + } else { + const auto& searchText = std::get(pattern); + matchFound = searchText.isEmpty() || header.contains(searchText, m_caseSensitivity); + } + } + + if (matchFound) + return true; + + if (m_payloadSearchEnabled) { + const auto payload = msg.toStringPayload(); + if (std::holds_alternative(pattern)) { + matchFound = payload.contains(std::get(pattern)); + } else { + const auto& searchText = std::get(pattern); + matchFound = payload.isEmpty() || payload.contains(searchText, m_caseSensitivity); + } + } + + return matchFound; +} + +bool DltMessageMatcher::matchAppId(const QString& appId) const +{ + return m_appId.isEmpty() || appId.compare(m_appId, m_caseSensitivity) == 0; +} + +bool DltMessageMatcher::matchCtxId(const QString& ctxId) const +{ + return m_ctxId.isEmpty() || ctxId.compare(m_ctxId, m_caseSensitivity) == 0; +} + +bool DltMessageMatcher::matchTimestampRange(unsigned int ts) const +{ + if (!m_timestampRange) + return true; + + // timestamp is displayed as floating number in UI and hence user provides timestamp ranges as floating numbers too + // in DltMsg stores timestamp as integer which is transformed to UI display floating number by QltMgs::toStringHeader + // method more or less as follows + const auto uiTs = static_cast(ts) / 10'000; + + return (m_timestampRange->start <= uiTs) && (uiTs <= m_timestampRange->end); +} diff --git a/qdlt/dltmessagematcher.h b/qdlt/dltmessagematcher.h new file mode 100644 index 00000000..88cf41b8 --- /dev/null +++ b/qdlt/dltmessagematcher.h @@ -0,0 +1,72 @@ +#ifndef DLTMESSAGEMATCHER_H +#define DLTMESSAGEMATCHER_H + +#include "export_rules.h" + +#include +#include + +#include +#include + +class QDltMsg; + +class QDLT_EXPORT DltMessageMatcher +{ +public: + using Pattern = std::variant; + + DltMessageMatcher(); + + void setCaseSentivity(Qt::CaseSensitivity caseSensitivity) { + m_caseSensitivity = caseSensitivity; + } + + void setSearchAppId(const QString& appId) { + m_appId = appId; + } + + void setSearchCtxId(const QString& ctxId) { + m_ctxId = ctxId; + } + + void setTimestapmRange(double start, double end) { + m_timestampRange = {start, end}; + } + + void setHeaderSearchEnabled(bool enabled) { + m_headerSearchEnabled = enabled; + } + + void setPayloadSearchEnabled(bool enabled) { + m_payloadSearchEnabled = enabled; + } + + void setMessageIdFormat(const QString& msgIdFormat) { + m_messageIdFormat = msgIdFormat; + } + + bool match(const QDltMsg& message, const Pattern& pattern) const; +private: + bool matchAppId(const QString& appId) const; + bool matchCtxId(const QString& ctxId) const; + bool matchTimestampRange(unsigned int ts) const; +private: + QString m_ctxId; + QString m_appId; + + struct TimestampRange { + double start; + double end; + }; + std::optional m_timestampRange; + + Qt::CaseSensitivity m_caseSensitivity{Qt::CaseInsensitive}; + + bool m_headerSearchEnabled{true}; + bool m_payloadSearchEnabled{true}; + + std::optional m_messageIdFormat; +}; + +#endif // DLTMESSAGEMATCHER_H diff --git a/qdlt/qdlt.pro b/qdlt/qdlt.pro index b1980c34..e51f5f40 100644 --- a/qdlt/qdlt.pro +++ b/qdlt/qdlt.pro @@ -2,7 +2,7 @@ PROJECT = qdlt TEMPLATE = lib -CONFIG += c++11 +CONFIG += c++1z DEFINES += QDLT_LIBRARY *-gcc* { QMAKE_CFLAGS += -std=gnu99 @@ -11,7 +11,7 @@ DEFINES += QDLT_LIBRARY } *-g++* { - QMAKE_CXXFLAGS += -std=gnu++0x + QMAKE_CXXFLAGS += -std=c++17 QMAKE_CXXFLAGS += -Wall QMAKE_CXXFLAGS += -Wextra QMAKE_CXXFLAGS += -DPLUGIN_INSTALLATION_PATH=\\\"$$PREFIX/usr/share/dlt-viewer/plugins\\\" @@ -65,6 +65,7 @@ SOURCES += \ qdltexporter.cpp \ fieldnames.cpp \ qdltimporter.cpp \ + dltmessagematcher.cpp \ HEADERS += qdlt.h \ export_rules.h \ @@ -96,6 +97,7 @@ HEADERS += qdlt.h \ qdltexporter.h \ fieldnames.h \ qdltimporter.h \ + dltmessagematcher.h \ unix:VERSION = 1.0.0 diff --git a/qdlt/tests/CMakeLists.txt b/qdlt/tests/CMakeLists.txt new file mode 100644 index 00000000..13b400a4 --- /dev/null +++ b/qdlt/tests/CMakeLists.txt @@ -0,0 +1,10 @@ +add_executable(test_tools + test_dltmessagematcher.cpp +) +target_link_libraries( + test_tools + PRIVATE + GTest::gtest_main + qdlt +) + diff --git a/qdlt/tests/test_dltmessagematcher.cpp b/qdlt/tests/test_dltmessagematcher.cpp new file mode 100644 index 00000000..ee035498 --- /dev/null +++ b/qdlt/tests/test_dltmessagematcher.cpp @@ -0,0 +1,122 @@ +#include + +#include "dltmessagematcher.h" +#include + +TEST(DltMessageMatcher, matchAppId) { + QDltMsg msg; + msg.setApid("Bla"); + + DltMessageMatcher matcher; + + // case insensitive search + matcher.setSearchAppId("bla"); + EXPECT_TRUE(matcher.match(msg, QString{})); + + // case sensitive search + matcher.setCaseSentivity(Qt::CaseSensitive); + EXPECT_FALSE(matcher.match(msg, QString{})); +} + +TEST(DltMessageMatcher, matchCtxId) { + QDltMsg msg; + msg.setCtid("Bla"); + + DltMessageMatcher matcher; + + // case insensitive search + matcher.setSearchCtxId("bla"); + EXPECT_TRUE(matcher.match(msg, QString{})); + + // case sensitive search + matcher.setCaseSentivity(Qt::CaseSensitive); + EXPECT_FALSE(matcher.match(msg, QString{})); +} + +TEST(DltMessageMatcher, matchTimestampRange) { + QDltMsg msg; + msg.setTimestamp(50000); + + + DltMessageMatcher matcher; + + // no timestamp range is set + EXPECT_TRUE(matcher.match(msg, QString{})); + + // in the range + matcher.setTimestapmRange(static_cast(msg.getTimestamp() - 10) / 10'000, + static_cast(msg.getTimestamp() + 10) / 10'000); + EXPECT_TRUE(matcher.match(msg, QString{})); + + // range is to the left + matcher.setTimestapmRange(static_cast(msg.getTimestamp() - 100) / 10'000, + static_cast(msg.getTimestamp() - 10) / 10'000); + EXPECT_FALSE(matcher.match(msg, QString{})); + + // range is to the right + matcher.setTimestapmRange(static_cast(msg.getTimestamp() + 10) / 10'000, + static_cast(msg.getTimestamp() + 100) / 10'000); + EXPECT_FALSE(matcher.match(msg, QString{})); +} + +TEST(DltMessageMatcher, matchMessageHeader) { + QDltMsg msg; + msg.setMicroseconds(4242); + msg.setTimestamp(45); + msg.setMessageCounter(2); + msg.setEcuid("ecuId"); + msg.setApid("appId"); + msg.setCtid("ctxId"); + msg.setSessionid(56); + msg.setType(QDltMsg::DltTypeNwTrace); + msg.setSubtype(3); + msg.setMode(QDltMsg::DltModeNonVerbose); + msg.setNumberOfArguments(255); + msg.setTime(123456789); + + DltMessageMatcher matcher; + matcher.setHeaderSearchEnabled(true); + matcher.setPayloadSearchEnabled(false); + + // simple text match + // empty header matches + EXPECT_TRUE(matcher.match(msg, "")); + EXPECT_TRUE(matcher.match(msg, "2 ecuId appId")); + EXPECT_FALSE(matcher.match(msg, "4243")); + + // regexp match + // simple text as regexp + EXPECT_TRUE(matcher.match(msg, QRegularExpression("ctxId"))); + // actual regexp: message starts with a date + EXPECT_TRUE(matcher.match(msg, QRegularExpression("^\\d\\d\\d\\d/\\d\\d/\\d\\d "))); + // actual regexp: somewhere in the middle there is "appId"-word separated from a next word with a space, + // at the end of the string there is a number + EXPECT_TRUE(matcher.match(msg, QRegularExpression("appId \\w+ .+\\d+$"))); +} + +TEST(DltMessageMatcher, matchMessagePayload) { + QDltMsg msg; + msg.setIndex(42); + msg.setEcuid("efgh"); + msg.setMode(QDltMsg::DltModeNonVerbose); + auto ba = QString{"abcd"}.toUtf8(); + msg.setPayload(ba); + // version number is set to make QDltMsg::toStringPayload produce string with payload + msg.setVersionNumber(2); + + DltMessageMatcher matcher; + matcher.setHeaderSearchEnabled(false); + matcher.setPayloadSearchEnabled(true); + + // simple text match + // empty header matches + EXPECT_TRUE(matcher.match(msg, "")); + EXPECT_FALSE(matcher.match(msg, "efgh")); + EXPECT_TRUE(matcher.match(msg, "abc")); + + //regexp match + // simple text as regexp + EXPECT_TRUE(matcher.match(msg, QRegularExpression("cd"))); + // actual regexp, match string "[0] abcd|61 62 63 64" + EXPECT_TRUE(matcher.match(msg, QRegularExpression("^\\[\\d\\]\\s+\\w+|[\\d\\s]+$"))); +} diff --git a/src/searchdialog.cpp b/src/searchdialog.cpp index 3e0d3386..0befd5a9 100644 --- a/src/searchdialog.cpp +++ b/src/searchdialog.cpp @@ -22,6 +22,8 @@ #include "mainwindow.h" #include "qdltoptmanager.h" +#include + #include #include #include @@ -389,8 +391,6 @@ void SearchDialog::findMessages(long int searchLine, long int searchBorder, QReg QDltMsg msg; QByteArray buf; - QString text; - QString headerText; int ctr = 0; Qt::CaseSensitivity is_Case_Sensitive = Qt::CaseInsensitive; @@ -401,9 +401,6 @@ void SearchDialog::findMessages(long int searchLine, long int searchBorder, QReg is_Case_Sensitive = Qt::CaseSensitive; } - - tempPayLoad.clear(); - is_PayloadStartFound = false; is_PayloadEndFound = false; is_PayLoadRangeValid = false; @@ -418,12 +415,23 @@ void SearchDialog::findMessages(long int searchLine, long int searchBorder, QReg bool msgIdEnabled=QDltSettingsManager::getInstance()->value("startup/showMsgId", true).toBool(); QString msgIdFormat=QDltSettingsManager::getInstance()->value("startup/msgIdFormat", "0x%x").toString(); + DltMessageMatcher matcher; + matcher.setCaseSentivity(is_Case_Sensitive); + matcher.setSearchAppId(stApid); + matcher.setSearchCtxId(stCtid); + if (is_TimeStampSearchSelected) { + matcher.setTimestapmRange(dTimeStampStart, dTimeStampStop); + } + if (msgIdEnabled) { + matcher.setMessageIdFormat(msgIdFormat); + } + matcher.setHeaderSearchEnabled(getHeader()); + matcher.setPayloadSearchEnabled(getPayload()); + do { ctr++; // for file progress indication - text.clear(); - if(getNextClicked() || searchtoIndex()) { searchLine++; @@ -464,187 +472,31 @@ void SearchDialog::findMessages(long int searchLine, long int searchBorder, QReg pluginManager->decodeMsg(msg, fSilentMode); } - headerText.clear(); - - /* search header */ - if( text.isEmpty() ) + const bool matchFound = getRegExp() ? matcher.match(msg, searchTextRegExp) : matcher.match(msg, getText()); + if (!matchFound) { - text += msg.toStringHeader(); - if ( msgIdEnabled==true ) - { - text += " "+QString::asprintf(msgIdFormat.toUtf8(),msg.getMessageId()); - } - tempPayLoad = msg.toStringPayload(); - - } // get the header text in case not empty - headerText = text; - if ( true == fIs_APID_CTID_requested ) - { - QString APID = headerText.section(" ",5,5); - QString CTID = headerText.section(" ",6,6); - // and check if the condition is valid - if ( ( APID.compare(stApid,is_Case_Sensitive) == 0 ) && ( stCtid.size() == 0 ) ) - { - // qDebug() << "APID hit" << searchLine << __LINE__; - } - else if ( ( CTID.compare(stCtid,is_Case_Sensitive) == 0 ) && ( stApid.size() == 0 ) ) - { - // qDebug() << "CTID hit" << searchLine << __LINE__; - } - else if( ( CTID.compare(stCtid,is_Case_Sensitive) == 0) && ( APID.compare(stApid,is_Case_Sensitive) == 0 ) ) - { - // qDebug() << "CTID & APID hit" << searchLine << __LINE__; - } - else - { - //qDebug() << APID << CTID << searchLine; - continue; // because if APID or CTID doesn not fit there is no need to search in any payload or header - } - } - - - is_TimeStampRangeValid = false; - if(true == is_TimeStampSearchSelected) // set the flag to identify a valid time stamp range - { - /*Assuming that the timeStamp is the 3rd value always*/ - QString TargetTimeStamp = headerText.section(" ",2,2); - if( ( dTimeStampStart <= TargetTimeStamp.toDouble() ) && ( dTimeStampStop >= TargetTimeStamp.toDouble() ) ) - { - //qDebug() << "Within time stamp range" << dTimeStampStart << TargetTimeStamp.toDouble() << dTimeStampStop << __LINE__; - is_TimeStampRangeValid = true; - } - else - continue; - } - - - if(getHeader() == true) // header is search enabled - { - if (getRegExp() == true) // regular expressions are selected - { - if(text.contains(searchTextRegExp)) - { - if ( foundLine(searchLine) ) - break; - else - continue; - } - else - { - //setMatch(false); - match = false; - } - } - else // no regular expressions search was requested - { - if(true == getText().isEmpty()) - { - if ( foundLine(searchLine) ) // so no pattern always fits - { - //qDebug() << "Header search hit in"<< __LINE__; - break; - } - else - continue; - } - else if(true == headerText.contains(getText(),is_Case_Sensitive)) // header search - { - { - if(true == timeStampPayloadValidityCheck(searchLine)) - break; - else - continue; - } - } - else // no fit, no display - { - match = false; - } - } - } // end header search - - /* search payload */ - text.clear(); - - if(getPayload() == true) // if payload is selected in the search box - { - if ( true == is_payLoadSearchSelected ) - { - if ( true == payLoadStartpatternCheck() ) // if payload pattern search range is set we try to detect the ranges - { - //qDebug() << "Found start payload pattern in " << searchLine << __LINE__; - } - } - - if( text.isEmpty()) - { - text += msg.toStringPayload(); - } + match = false; + continue; + } - if (getRegExp() == true) - { - if(tempPayLoad.contains(searchTextRegExp)) - { - if ( foundLine(searchLine) ) - { - //qDebug() << "Search hit in"<< __LINE__; - break; - } - else - { - payLoadStoppatternCheck(); - continue; - } - } - else - { - //setMatch(false); - match = false; - } - } - else // search option without regular expressions - { - if(getText().isEmpty() == true) // no search text for payload given - { - if(timeStampPayloadValidityCheck(searchLine)) - { - break; - } - else - { - payLoadStoppatternCheck(); - continue; - } + // TODO: implement functionality about payload start and end + // Note: This feature has been broken for some time before this refactoring: + // See https://github.com/COVESA/dlt-viewer/issues/502 - } - else if(tempPayLoad.contains(getText(),is_Case_Sensitive)) - { - if(timeStampPayloadValidityCheck(searchLine)) - { - break; - } - else - { - payLoadStoppatternCheck(); - continue; - } - } - else - { - match = false; - } - } - } + if (foundLine(searchLine)) + break; + else + continue; } while( searchBorder != searchLine ); stoptime(); } -bool SearchDialog::payLoadStartpatternCheck() +bool SearchDialog::payLoadStartpatternCheck(const QString& tempPayload) { // When the start payload patternn is found, consider range as valid - if((tempPayLoad.contains(payloadStart)) && (false == is_PayloadEndFound) && true == is_payLoadSearchSelected) + if((tempPayload.contains(payloadStart)) && (false == is_PayloadEndFound) && true == is_payLoadSearchSelected) { //qDebug() << "Found start payload pattern" << __LINE__; is_PayLoadRangeValid = true; @@ -654,12 +506,12 @@ bool SearchDialog::payLoadStartpatternCheck() } -bool SearchDialog::payLoadStoppatternCheck() +bool SearchDialog::payLoadStoppatternCheck(const QString& tempPayload) { // When the stop payload patern is found, consider range as ivalid if(true == is_PayloadStartFound && true == is_payLoadSearchSelected && true == is_payLoadSearchSelected) { - if(tempPayLoad.contains(payloadEnd)) + if(tempPayload.contains(payloadEnd)) { //qDebug() << "Found stop payload pattern" << __LINE__; is_PayloadEndFound = true; @@ -675,24 +527,21 @@ bool SearchDialog::timeStampPayloadValidityCheck(long int searchLine) // qDebug() << "timeStampPayloadValidityCheck" << __LINE__; if(true == is_TimeStampSearchSelected) { - if(true == is_TimeStampRangeValid) + if(true == is_payLoadSearchSelected) { - if(true == is_payLoadSearchSelected) + if(is_PayLoadRangeValid == true) { - if(is_PayLoadRangeValid == true) + if(foundLine(searchLine)) { - if(foundLine(searchLine)) - { - return true; - } + return true; } } - else + } + else + { + if(foundLine(searchLine) == true) { - if(foundLine(searchLine) == true) - { - return true; - } + return true; } } } diff --git a/src/searchdialog.h b/src/searchdialog.h index 3891a278..7723a0e8 100644 --- a/src/searchdialog.h +++ b/src/searchdialog.h @@ -89,7 +89,6 @@ class SearchDialog : public QDialog bool is_PayLoadRangeValid; bool is_payLoadSearchSelected; bool is_TimeStampSearchSelected; - bool is_TimeStampRangeValid; bool fIs_APID_CTID_requested; QString TimeStampStarttime; @@ -99,7 +98,6 @@ class SearchDialog : public QDialog QString payloadStart; QString payloadEnd; - QString tempPayLoad; QString stApid; QString stCtid; @@ -133,8 +131,8 @@ class SearchDialog : public QDialog bool getOnceClicked(); bool searchtoIndex(); bool foundLine(long int searchLine); - bool payLoadStartpatternCheck(); - bool payLoadStoppatternCheck(); + bool payLoadStartpatternCheck(const QString& tempPayload); + bool payLoadStoppatternCheck(const QString& tempPayload); QString getApIDText(); QString getCtIDText(); QString getTimeStampStart(); diff --git a/src/src.pro b/src/src.pro index ea113ce1..e79c94df 100644 --- a/src/src.pro +++ b/src/src.pro @@ -4,6 +4,7 @@ QT_VERSION = $$split(QT_VERSION, ".") QT_VER_MAJ = $$member(QT_VERSION, 0) QT_VER_MIN = $$member(QT_VERSION, 1) +CONFIG += c++1z *-gcc* { QMAKE_CFLAGS += -std=gnu99 QMAKE_CFLAGS += -Wall @@ -12,7 +13,7 @@ QT_VER_MIN = $$member(QT_VERSION, 1) } *-g++* { - QMAKE_CXXFLAGS += -std=gnu++0x + QMAKE_CXXFLAGS += -std=c++17 QMAKE_CXXFLAGS += -Wall QMAKE_CXXFLAGS += -Wextra #QMAKE_CXXFLAGS += -pedantic