Skip to content

Conversation

minggangw
Copy link
Member

@minggangw minggangw commented Jun 11, 2025

This PR adds support for starting the type description service via a configurable parameter and includes a new test case to verify that configuration.

  • Added a new test case in test/test-type-description-service.js to check the parameter configuration.
  • Updated lib/type_description_service.js to declare and retrieve the "start_type_description_service" parameter and conditionally start the service.

Fix: #1161

@minggangw minggangw requested a review from Copilot June 11, 2025 09:31
Copilot

This comment was marked as outdated.

@minggangw minggangw requested a review from Copilot June 11, 2025 09:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new ROS2 parameter to control whether the type description service is started and includes a test to verify that parameter is registered.

  • Declare and retrieve a start_type_description_service boolean parameter in TypeDescriptionService and conditionally start the service.
  • Add a test that shells out to ros2 param list to confirm the new parameter is present on the node.

Reviewed Changes

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

File Description
test/test-type-description-service.js Import exec and add test to verify start_type_description_service appears in parameter list.
lib/type_description_service.js Declare/retrieve the new parameter and update start() to respect the parameter’s value.
Comments suppressed due to low confidence (2)

lib/type_description_service.js:32

  • [nitpick] Add a note in the class-level JSDoc explaining the new 'start_type_description_service' parameter and its default behavior.
// This class is used to create a TypeDescriptionService which can be used to

test/test-type-description-service.js:78

  • Add a complementary test case to verify that when start_type_description_service is set to false, the service does not start.
  it('Test type description service configured by parameter', function (done) {

Comment on lines +43 to +59
const startTypeDescriptionServiceParam = 'start_type_description_service';
if (!node.hasParameter(startTypeDescriptionServiceParam)) {
node.declareParameter(
new Parameter(
startTypeDescriptionServiceParam,
ParameterType.PARAMETER_BOOL,
true
),
new ParameterDescriptor(
startTypeDescriptionServiceParam,
ParameterType.PARAMETER_BOOL,
'If enabled, start the ~/get_type_description service.',
true
)
);
}
const param = node.getParameter(startTypeDescriptionServiceParam);
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the parameter name 'start_type_description_service' into a top-level constant to avoid duplication and reduce risk of typos.

Suggested change
const startTypeDescriptionServiceParam = 'start_type_description_service';
if (!node.hasParameter(startTypeDescriptionServiceParam)) {
node.declareParameter(
new Parameter(
startTypeDescriptionServiceParam,
ParameterType.PARAMETER_BOOL,
true
),
new ParameterDescriptor(
startTypeDescriptionServiceParam,
ParameterType.PARAMETER_BOOL,
'If enabled, start the ~/get_type_description service.',
true
)
);
}
const param = node.getParameter(startTypeDescriptionServiceParam);
if (!node.hasParameter(START_TYPE_DESCRIPTION_SERVICE_PARAM)) {
node.declareParameter(
new Parameter(
START_TYPE_DESCRIPTION_SERVICE_PARAM,
ParameterType.PARAMETER_BOOL,
true
),
new ParameterDescriptor(
START_TYPE_DESCRIPTION_SERVICE_PARAM,
ParameterType.PARAMETER_BOOL,
'If enabled, start the ~/get_type_description service.',
true
)
);
}
const param = node.getParameter(START_TYPE_DESCRIPTION_SERVICE_PARAM);

Copilot uses AI. Check for mistakes.

Comment on lines +79 to +97
exec(
'ros2 param list /test_type_description_service',
(error, stdout, stderr) => {
if (error || stderr) {
done(
new Error(
'Test type description service configured by parameter failed.'
)
);
}
if (stdout.includes('start_type_description_service')) {
done();
} else {
done(
new Error("'start_type_description_service' not found in stdout.")
);
}
}
);
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shelling out to ros2 in tests can be brittle; consider using the rclnodejs Node API to list or get parameters directly in code for a more reliable test.

Suggested change
exec(
'ros2 param list /test_type_description_service',
(error, stdout, stderr) => {
if (error || stderr) {
done(
new Error(
'Test type description service configured by parameter failed.'
)
);
}
if (stdout.includes('start_type_description_service')) {
done();
} else {
done(
new Error("'start_type_description_service' not found in stdout.")
);
}
}
);
const parameterName = 'start_type_description_service';
const parameters = node.getParameters([parameterName]);
if (parameters[parameterName] !== undefined) {
done();
} else {
done(
new Error(`'${parameterName}' not found in node parameters.`)
);
}

Copilot uses AI. Check for mistakes.

Comment on lines +82 to +88
if (error || stderr) {
done(
new Error(
'Test type description service configured by parameter failed.'
)
);
}
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Currently the test fails on any stderr output; consider only failing on a non-zero exit code and logging stderr to avoid false negatives from warnings.

Suggested change
if (error || stderr) {
done(
new Error(
'Test type description service configured by parameter failed.'
)
);
}
if (error) {
done(
new Error(
`Test type description service configured by parameter failed with error: ${error.message}`
)
);
return;
}
if (stderr) {
console.warn(`Warning: stderr output detected: ${stderr}`);
}

Copilot uses AI. Check for mistakes.

@coveralls
Copy link

coveralls commented Jun 11, 2025

Coverage Status

coverage: 84.711% (+0.09%) from 84.625%
when pulling 341f936 on minggangw:fix-1161
into b3cf4a7 on RobotWebTools:develop.

@minggangw minggangw merged commit 4cff06a into RobotWebTools:develop Jun 12, 2025
16 checks passed
minggangw added a commit that referenced this pull request Jun 12, 2025
This PR adds support for starting the type description service via a configurable parameter and includes a new test case to verify that configuration.  
- Added a new test case in test/test-type-description-service.js to check the parameter configuration.  
- Updated lib/type_description_service.js to declare and retrieve the "start_type_description_service" parameter and conditionally start the service.

Fix: #1161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable get_type_description service via parameter

2 participants