test: add test-grpc-reconfigure to agent tests#298
test: add test-grpc-reconfigure to agent tests#298santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
Conversation
WalkthroughThe changes introduce support for a "reconfigure" command in the gRPC agent testing infrastructure. A new test suite is added to verify the reconfiguration functionality, including validation of configuration updates and message structures. The Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant GRPCServer
participant GRPCAgentServer
TestSuite->>GRPCServer: reconfigure(agentId, config)
GRPCServer->>GRPCAgentServer: Send 'reconfigure' message (with requestId, agentId, config)
GRPCAgentServer->>GRPCServer: Process and respond with 'reconfigure' message (matching requestId)
GRPCServer->>TestSuite: Resolve promise with response data
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/agents/test-grpc-reconfigure.mjs (1)
16-63: Improve the string number detection in checkReconfigureDataThe function correctly validates the reconfigure message format and normalizes configuration objects for comparison. However, the string number detection logic could be improved.
// Convert string numbers to actual numbers for comparison - if (!Number.isNaN(value) && typeof value === 'string') { + if (typeof value === 'string' && !isNaN(value)) { normalizedReconfigBody[key] = parseInt(value, 10); } else { normalizedReconfigBody[key] = value; }
Number.isNaN()doesn't perform type conversion, so it will always return false for string values. UsingisNaN()(without the Number prefix) will attempt to convert the value to a number first, which is the behavior you want for detecting numeric strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/agents/test-grpc-reconfigure.mjs(1 hunks)test/common/nsolid-grpc-agent/index.js(2 hunks)test/common/nsolid-grpc-agent/server.mjs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-linux-without-intl
- GitHub Check: lint-cpp
- GitHub Check: build-docs
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
- GitHub Check: build-tarball
- GitHub Check: test-macOS
🔇 Additional comments (8)
test/common/nsolid-grpc-agent/index.js (1)
112-114: LGTM - Added handler for the new 'reconfigure' message typeThe implementation follows the established pattern used for other message types in the switch statement. The event is correctly emitted with the message data.
test/common/nsolid-grpc-agent/server.mjs (3)
184-184: LGTM - Implemented reconfigure message handlingThe implementation correctly sends the reconfigure message data to the parent process, following the pattern established for other message types.
237-239: LGTM - Added handler for reconfigure messagesThe implementation correctly routes reconfigure messages to the new sendReconfigure function, following the pattern established for other message types.
317-323: LGTM - Implemented sendReconfigure functionThe function follows the established pattern for other command functions. It correctly structures the arguments and calls the common sendCommand utility function.
test/agents/test-grpc-reconfigure.mjs (4)
67-102: LGTM - Test for retrieving current configurationThe test properly verifies that the reconfigure command returns the current configuration when no changes are specified. It follows good practices:
- Starts a gRPC server and test client
- Retrieves the agent ID
- Sends a reconfigure request without changes
- Verifies the response against the current configuration
- Properly cleans up resources
104-119: LGTM - Comprehensive set of test configurationsThe test data covers a good range of configuration types:
- Numeric values (like thresholds and intervals)
- Boolean flags
- Strings
- Arrays
- Template strings
This provides thorough coverage of the various configuration types that might be updated.
120-184: LGTM - Test for applying valid configuration changesThe test correctly verifies that the reconfigure command properly applies and returns valid configuration changes. It:
- Iterates through multiple configuration updates sequentially
- Verifies each update is correctly applied
- Handles string-to-number conversion
- Properly cleans up resources when done
The recursive approach to sending configs sequentially is clean and effective.
186-189: LGTM - Test execution logicThe implementation correctly runs all tests sequentially and logs their names for better visibility in test output.
santigimeno
force-pushed
the
santi/add_reconfigure_test
branch
from
5c2752a to
367b56d
Compare
santigimeno
force-pushed
the
santi/add_reconfigure_test
branch
from
367b56d to
6b63c24
Compare
PR-URL: #298 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
|
Landed in ed6ddc5 |
PR-URL: #298 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #298 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #298 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
2 participants
Summary by CodeRabbit
New Features
Tests