Fix clang-tidy noise from bundled headers and flaky secured-test deadline#495
Closed
mfaferek93 wants to merge 4 commits into
Closed
Fix clang-tidy noise from bundled headers and flaky secured-test deadline#495mfaferek93 wants to merge 4 commits into
mfaferek93 wants to merge 4 commits into
Conversation
…r filter The quality job's -header-filter='.*ros2_medkit.*' matches vendored headers because their install paths contain the package name, so any TU including them (e.g. via lock_manager.hpp) fails with third-party warnings. Wrap them in NOLINTBEGIN/NOLINTEND.
On slow CI runners the fired alarm does not always propagate through report -> fault_manager -> REST within 40s. Raise the wait to 120s and the CTest timeout to 420s so a failing run still prints diagnostics. Passing runs exit the poll early and are unaffected.
Only open62541pp's own interface dirs were marked SYSTEM; the bundled open62541 target's dirs stayed plain -I, so including a header like client_highlevel.h turns its old-style casts into hard clang-tidy errors via -Werror=old-style-cast.
Complete the special-member sets of OpcuaClient, OpcuaPlugin and OpcuaPoller with deleted moves, drop a no-effect std::move on the trivially copyable Subscription handle, build event-field Variants via the deep-copy constructor, and convert an index loop to range-for.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces CI noise/failures by suppressing clang-tidy diagnostics from vendored/third-party headers and by fixing genuine clang-tidy findings in the OPC UA plugin code, while also making the secured OPC UA integration test less flaky under slow runners.
Changes:
- Suppress clang-tidy on vendored headers that match the CI header filter using
NOLINTBEGIN/NOLINTEND. - Adjust the OPC UA plugin build to treat both
open62541ppand bundledopen62541interface include directories asSYSTEM, and fix several clang-tidy findings in OPC UA source/header code. - Increase the secured OPC UA integration test’s internal wait deadline and raise the corresponding CTest timeout.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/yaml_utils.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/vector_utils.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/typesupport.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/types.h | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/string_utils.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/msg_parser.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/message_reading.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_serialization/include/ros2_medkit_serialization/vendored/dynmsg/config.hpp | Add clang-tidy suppression markers for vendored dynmsg header. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_secured.test.py | Increase secured-test fault propagation wait deadline and document the rationale. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp | Address clang-tidy findings (redundant move, safer variant deep-copy, range-for cleanup). |
| src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp | Explicitly delete move operations to satisfy special-member-functions guidance. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp | Explicitly delete copy/move operations to satisfy special-member-functions guidance. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp | Explicitly delete move operations to satisfy special-member-functions guidance. |
| src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt | Mark third-party include dirs as SYSTEM and increase secured integration test TIMEOUT. |
| src/ros2_medkit_gateway/src/vendored/tl_expected/include/tl/expected.hpp | Add clang-tidy suppression markers for vendored tl::expected header. |
Comment on lines
+358
to
+364
| # TIMEOUT must exceed the sum of the script's internal wait deadlines so a | ||
| # slow failing run still reaches its own FAIL diagnostics instead of being | ||
| # killed by CTest. | ||
| set_tests_properties(test_opcua_secured PROPERTIES | ||
| LABELS "integration" | ||
| SKIP_RETURN_CODE 77 | ||
| TIMEOUT 300) | ||
| TIMEOUT 420) |
Collaborator
Author
|
Folded into #492. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the red clang-tidy job on PRs touching the OPC UA plugin, and the flaky secured integration test. Four independent commits:
#include <open62541/client_highlevel.h>surfaced hard-Werror=old-style-casterrors from the third-party headers, because only open62541pp's dirs were SYSTEM while the bundled open62541 target's were plain-I. Verified with a probe TU: before = error flood from the vendor headers, after = only the TU's own deliberate violation is reported.tl/expected.hpp, the dynmsg headers) - their install paths contain "ros2_medkit". Verified the suppression does not leak: a deliberate violation in the including TU is still reported.std::moveon trivially-copyable handles, one range-for). Note: the event-field case was not a plain move-drop - the manualUA_Variant_copy+ adopt was replaced with a direct deep-copyopcua::Variantconstruction to avoid leaking the copied buffer.Tested: clang-tidy with the CI filter reports 0 findings on the touched sources; OPC UA suite 222 tests pass; flake8 clean on the test script.