Test Coverage and mruby logging

[wsobel]

Added additional tests
Fixed mruby logging issues
Added more logging in the agent adapter

[Copilot]

Pull request overview

This PR expands automated test coverage around topic/message mapping, HTTP swagger rendering, agent behavior, and configuration parsing, while also improving logging and robustness in the adapter pipeline and Ruby integration.

Changes:

• Added/expanded pipeline- and mapping-focused tests (topic mapping, message mapping, agent behavior, HTTP swagger rendering, adapter URL parsing).
• Improved logging and diagnostics (agent adapter lifecycle/recovery logs, mruby logger integration, richer Ruby transform error output).
• Small API/utility adjustments (URL formatting const-correctness + stream operator, schema version propagation during config load, safer error logging in DataMapper).

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test_package/topic_mapping_test.cpp	Adds TopicMapper-focused helpers and new mapping/cache/path-scan tests.
test_package/message_mapping_test.cpp	New DataMapper tests covering resolved/unresolved mapping and non-string value behavior.
test_package/http_server_test.cpp	Adds Accept header support in test client and swagger rendering/allowPutFrom tests.
test_package/config_test.cpp	Adds adapter URL parsing test coverage and malformed URL rejection test.
test_package/CMakeLists.txt	Registers new message_mapping test target.
test_package/agent_test.cpp	Adds tests for getDeviceByName const behavior and source failure handling.
src/mtconnect/utilities.hpp	Makes Url::getUrlText const and adds operator<< for Url.
src/mtconnect/source/adapter/agent_adapter/agent_adapter.cpp	Adds additional lifecycle/recovery logging to agent adapter.
src/mtconnect/sink/rest_sink/rest_service.cpp	Fixes typo “stapshot” → “snapshot” in REST routing documentation.
src/mtconnect/ruby/ruby_vm.hpp	Updates mruby logging to use BOOST_LOG_SEV with the agent logger.
src/mtconnect/ruby/ruby_transform.hpp	Adds additional logging when Ruby transform execution fails.
src/mtconnect/ruby/ruby_observation.hpp	Adds detailed Ruby observation wrapper documentation/notes.
src/mtconnect/pipeline/message_mapper.hpp	Makes error logging type-safe for non-string message values.
src/mtconnect/configuration/agent_config.hpp	Exposes expandConfigVariables publicly and documents behavior.
src/mtconnect/configuration/agent_config.cpp	Runs config variable expansion earlier and adjusts schema version option handling.
src/mtconnect/agent.cpp	Attempts to derive schema version from the device XML when not explicitly provided.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

test_package/topic_mapping_test.cpp:124

• makeDeviceOnlyDataItem uses EXPECT_NE to validate the device exists, but then immediately dereferences dev->second. If the device is missing, this is undefined behavior (the test continues after a failed EXPECT). Use ASSERT_NE here to abort the test before dereferencing.

    auto dev = m_devices.find(device);
    EXPECT_NE(m_devices.end(), dev) << "Cannot find device: " << device;

test_package/http_server_test.cpp:104

• m_accept is documented as applying to “the next request”, but request() never clears it after setting the header. This makes the Accept header sticky across subsequent requests and can cause cross-test/request coupling. Clear m_accept after applying it (or rename it to indicate it is persistent).

    if (!m_accept.empty())
      req.set(http::field::accept, m_accept);

src/mtconnect/source/adapter/agent_adapter/agent_adapter.cpp:244

• Typo in log message: "Attmempting" -> "Attempting".

              LOG(info) << "Attmempting to recover stream for " << self->m_url;

src/mtconnect/configuration/agent_config.cpp:981

• This logic overwrites an explicitly configured SchemaVersion option: when HasOption(SchemaVersion) is true, execution falls into the else branch and resets the option to the agent version. This is a breaking behavior change for configuration. Only set SchemaVersion when it is not already present.

    if (!HasOption(options, configuration::SchemaVersion) &&
        m_agent->getSchemaVersion())
    {
      options[configuration::SchemaVersion] = *m_agent->getSchemaVersion();
    }

src/mtconnect/utilities.hpp:1017

• The Doxygen for operator<< refers to “Value” and parameter v, but the overload is for Url and takes url. This makes generated docs misleading.

    /// @brief output operator for Value
    /// @param os the output stream
    /// @param v the Value to output
    inline std::ostream &operator<<(std::ostream &os, const Url &url)

src/mtconnect/agent.cpp:132

• This adds an extra m_xmlParser->parseFile() in the constructor to derive m_schemaVersion, but initialize() already parses the same devices file via loadXMLDeviceFile() (and sets m_schemaVersion there). When SchemaVersion isn’t configured, this results in parsing the devices file twice during startup.

    if (!m_schemaVersion)
    {
      m_xmlParser->parseFile(m_deviceXmlPath, dynamic_cast<printer::XmlPrinter *>(m_printers["xml"].get()));
      m_schemaVersion = m_xmlParser->getSchemaVersion();
    }

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mtconnect/ruby/ruby_vm.hpp:69

• ruby_vm.hpp now logs via agent_logger/BOOST_LOG_SEV, but this header does not include mtconnect/logging.hpp (or the required Boost.Log headers). That introduces an include-order dependency and can fail to compile in translation units that include ruby_vm.hpp without already including the logging headers.

      BOOST_LOG_SEV(agent_logger::get(), level) <<  mrb_str_to_cstr(mrb, msg);

test_package/message_mapping_test.cpp:2

• The file header contains a duplicated word ("Copyright Copyright"). This looks like a copy/paste typo and should be corrected to avoid propagating it to new files.

// Copyright Copyright 2009-2025, AMT – The Association For Manufacturing Technology (“AMT”)

src/mtconnect/configuration/agent_config.cpp:988

• This uses std::format but this .cpp does not include , which can break the build depending on transitive includes/toolchain. Since this is just constructing ".", simple concatenation avoids the extra header dependency.

        options[configuration::SchemaVersion] = std::format("{}.{}", std::to_string(AGENT_VERSION_MAJOR),
                                                            std::to_string(AGENT_VERSION_MINOR));
        
      }

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

test_package/message_mapping_test.cpp:116

• makeDataItem uses ASSERT_NE inside a non-void helper. Like other gtest ASSERT_* macros, this can expand to return; on failure, which is not valid in a function returning DataItemPtr and can cause compile errors. Prefer a non-fatal check with an explicit return nullptr.
  test_package/message_mapping_test.cpp:58
• MockPipelineContract::findDevice / findDataItem use std::map::operator[], which inserts a default-constructed (null) entry when the key is missing. That mutates the test fixture state and can mask lookup failures. Use find() and return nullptr when not found.

This issue also appears on line 112 of the same file.

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (5)

test_package/message_mapping_test.cpp:33

• This helper depends on using namespace std; and a specific include order to compile because mtconnect/pipeline/message_mapper.hpp currently uses unqualified string (and references adapter Handler types) without providing the required declarations itself. Encoding this workaround in a new test makes the test brittle; it would be better to make message_mapper.hpp self-contained (qualify std::string and include/forward-declare needed types) and then remove the workaround here.
  test_package/topic_mapping_test.cpp:132
• DataItem::make() can return nullptr (and/or populate errors). This helper adds the returned pointer to the device without asserting creation succeeded, which can turn a config/test issue into a null dereference or an unhelpful downstream failure. Add assertions for di and errors before calling addDataItem().
  test_package/message_mapping_test.cpp:120
• DataItem::make() can return nullptr (and/or populate errors). This helper immediately dereferences di for its id and stores it, which can crash the test binary if DataItem creation fails. Add assertions for di and errors before using it.
  src/mtconnect/agent.cpp:131
• When SchemaVersion is not provided via options, the constructor now parses the device XML solely to extract the schema version, but the same file is parsed again during initialize() via loadXMLDeviceFile(). This double-parse is avoidable and adds startup overhead. Consider deferring schema-version detection to loadXMLDeviceFile() (where the parse is already required) and only setting printers’ schema version after that parse completes.

    if (!m_schemaVersion)
    {
      m_xmlParser->parseFile(m_deviceXmlPath, dynamic_cast<printer::XmlPrinter *>(m_printers["xml"].get()));
      m_schemaVersion = m_xmlParser->getSchemaVersion();
    }

test_package/http_server_test.cpp:1099

• This test uses a DNS-style hostname to exercise the failure path of Server::allowPutFrom(), but allowPutFrom() performs a resolver lookup (src/mtconnect/sink/rest_sink/server.cpp:165-171). Depending on the CI runner’s resolver configuration, this can introduce long timeouts/flakiness. Prefer an immediately-invalid hostname so resolution fails fast without DNS/network dependencies.

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (4)

test_package/message_mapping_test.cpp:124

• makeDataItem() assumes DataItem::make() always returns a valid pointer and immediately dereferences it (getId()) and passes it to addDataItem(). If DataItem::make() fails and returns nullptr (or reports errors), this will crash the test binary.
  test_package/topic_mapping_test.cpp:132
• makeDeviceOnlyDataItem() calls DataItem::make() and then unconditionally passes the result to addDataItem(). DataItem::make() can return nullptr (and/or populate errors), which would make this a potential null dereference inside addDataItem().
  test_package/message_mapping_test.cpp:33
• This test needs an unusual include order plus a global using namespace std; to compile message_mapper.hpp (see comment). That strongly suggests message_mapper.hpp is not self-contained (unqualified string usage / missing includes or forward declarations), and the workaround risks leaking std symbols into other includes in this TU.
  test_package/message_mapping_test.cpp:58
• MockPipelineContract::findDevice/findDataItem use operator[] on the maps, which mutates the maps by inserting missing keys. That can hide lookup bugs and make tests pass unintentionally (missing keys become present with nullptr values). Prefer a non-mutating lookup and return nullptr when not found.

This issue also appears on line 112 of the same file.

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mtconnect/mqtt/mqtt_server_impl.hpp:40

• using namespace std; was removed from this header, but the file still uses unqualified string (e.g., GetOption<string>(...) and a lambda returning -> string). As-is, this will not compile unless a consumer happens to inject std::string into scope. Either fully-qualify those occurrences or add a narrow using std::string; in this namespace.
  test_package/message_mapping_test.cpp:58
• MockPipelineContract::findDevice / findDataItem use std::map::operator[], which inserts a new (null) entry on misses. That can mask lookup problems in tests and mutates fixture state unexpectedly. Prefer find() and return nullptr when not found.
  src/mtconnect/agent.cpp:131
• This constructor now parses the device XML via m_xmlParser->parseFile(...) to derive m_schemaVersion, but initialize() later calls loadXMLDeviceFile() which parses the same file again. This duplicates potentially expensive XML parsing work and can introduce duplicated side-effects from parsing. Consider refactoring so schema version can be derived without a full parse, or cache/reuse the first parse result so the device XML is only parsed once.

    if (!m_schemaVersion)
    {
      m_xmlParser->parseFile(m_deviceXmlPath, dynamic_cast<printer::XmlPrinter *>(m_printers["xml"].get()));
      m_schemaVersion = m_xmlParser->getSchemaVersion();
    }

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mtconnect/mqtt/mqtt_server_impl.hpp:40

• Removing using namespace std; means this header now has several unqualified string uses later (e.g., the TLS password callback return type and GetOption<string>(...)), which will fail to compile. Either qualify those as std::string throughout or add a narrow using std::string; in the namespace scope.
  test_package/message_mapping_test.cpp:58
• findDevice/findDataItem use map::operator[], which inserts missing keys and mutates the maps during lookups. This can hide test setup issues and affect later assertions. Prefer find() and return nullptr on misses without side effects.

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mtconnect/mqtt/mqtt_server_impl.hpp:40

• The file no longer has using namespace std;, but it still uses unqualified string (e.g., TLS password callback return type) and GetOption<string> later in the file. Without std:: qualification (or a local alias), this will not compile.
  src/mtconnect/agent.cpp:131
• This constructor now parses the device XML (m_xmlParser->parseFile) solely to obtain the schema version, but initialize() immediately parses the same file again via loadXMLDeviceFile(). This guarantees the device XML is parsed twice on startup, which can significantly increase agent startup time for large device files.

    if (!m_schemaVersion)
    {
      m_xmlParser->parseFile(m_deviceXmlPath, dynamic_cast<printer::XmlPrinter *>(m_printers["xml"].get()));
      m_schemaVersion = m_xmlParser->getSchemaVersion();
    }

[Copilot]

Pull request overview

Copilot reviewed 285 out of 285 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mtconnect/configuration/agent_config.cpp:982

• SchemaVersion is being injected into options after the Agent is constructed, so the Agent will not see this value (it copies options in its constructor). Also, m_agent->getSchemaVersion() is read before m_agent->initialize(...), so it will typically be empty and this block will fall back even when the device XML schema version could differ. Consider setting the default SchemaVersion before constructing the Agent so the Agent and sinks are configured consistently.
  test_package/message_mapping_test.cpp:58
• MockPipelineContract::findDevice / findDataItem use std::map::operator[], which will insert a default-constructed entry on lookup misses. That side effect can hide errors in tests and can also mutate the maps in ways that affect later expectations.
  src/mtconnect/mqtt/mqtt_server_impl.hpp:360
• The log message string contains a non-printable control character after "Certificates". This can make logs hard to read and may cause issues in some terminals/log processors.

[Copilot]

Pull request overview

Copilot reviewed 286 out of 286 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/mtconnect/mqtt/mqtt_server_impl.hpp:361

• This log message contains a stray control character (looks like U+0013) in the string literal (between "Certificates" and "."). It will make logs hard to search/copy and can cause encoding issues depending on the sink.
  test_package/message_mapping_test.cpp:58
• MockPipelineContract::findDevice and findDataItem use std::map::operator[], which will insert a new entry with a default-constructed (null) value when the key is missing. That mutates test state during lookups and can mask missing-device/data-item bugs.

Prefer find() and return nullptr when not present.

[Copilot]

Pull request overview

Copilot reviewed 286 out of 286 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mtconnect/pipeline/message_mapper.hpp:51

• DataMapper's constructor takes source::adapter::Handler*, but this header does not declare Handler (nor include the header that defines it). This makes message_mapper.hpp non-self-contained and forces include-order workarounds in users/tests.
  test_package/conanfile.py:10
• Duplicate/unused imports at the top of this recipe make it harder to maintain (and can confuse linters). os is imported twice and isn't used in this file.