Support hyphenated ROSIDL subdirs (#1390)

This PR adds support for ROSIDL packages with hyphenated subdirectory names (e.g., `msg-common`, `msg-ros2`) by updating the subfolder extraction logic to normalize these to their base interface types (`msg`, `srv`, `action`).

**Changes:**
- Modified `getSubFolder()` to extract base interface type from hyphenated subdirectory names using `split('-')[0]`
- Added comprehensive test case using `mrpt_msgs` package to validate hyphenated subfolder support
- Refactored test setup in `test-native-loader.js` to avoid repeated module reloading
- Updated CI workflows to install `mrpt-msgs` package for testing
- Simplified process exit handling in test runner script

Fix: #1388

Author: minggangw

.github/workflows/linux-arm64-build-and-test.yml

@@ -49,9 +49,9 @@ jobs:
       with:
         required-ros-distributions: ${{ matrix.ros_distribution }}
 
-    - name: Install test-msgs on Linux
+    - name: Install test-msgs and mrpt_msgs on Linux
       run: |
-        sudo apt install ros-${{ matrix.ros_distribution }}-test-msgs
+        sudo apt install ros-${{ matrix.ros_distribution }}-test-msgs ros-${{ matrix.ros_distribution }}-mrpt-msgs
         # Adjust dependencies based on Ubuntu version
         LIBASOUND_PKG="libasound2"
         if grep -q "24.04" /etc/os-release; then

.github/workflows/linux-x64-build-and-test.yml

@@ -53,9 +53,9 @@ jobs:
         required-ros-distributions: ${{ matrix.ros_distribution }}
         use-ros2-testing: ${{ matrix.ros_distribution == 'rolling' }}
 
-    - name: Install test-msgs on Linux
+    - name: Install test-msgs and mrpt_msgs on Linux
       run: |
-        sudo apt install ros-${{ matrix.ros_distribution }}-test-msgs
+        sudo apt install ros-${{ matrix.ros_distribution }}-test-msgs ros-${{ matrix.ros_distribution }}-mrpt-msgs
         # Adjust dependencies based on Ubuntu version
         LIBASOUND_PKG="libasound2"
         if grep -q "24.04" /etc/os-release; then

rosidl_gen/packages.js

@@ -52,7 +52,12 @@ function getSubFolder(filePath, amentExecuted) {
   }
 
   if (amentExecuted) {
-    return filePath.match(/\w+\/share\/\w+\/(\w+)\//)[1];
+    const match = filePath.match(/\w+\/share\/\w+\/([\w-]+)\//);
+    if (match) {
+      // Handle non-standard subfolder names (e.g., msg-common, msg-ros2)
+      // by extracting only the base interface type before any hyphen.
+      return match[1].split('-')[0];
+    }
   }
   // If the |amentExecuted| equals to false, the file's extension will be assigned as
   // the name of sub folder.

scripts/run_test.js

@@ -43,9 +43,8 @@ utils
     });
 
     mocha.run(function (failures) {
-      process.on('exit', () => {
-        process.exit(failures);
-      });
+      process.exitCode = failures ? 1 : 0;
+      process.exit(process.exitCode);
     });
   })
   .catch((err) => {

test/test-native-loader.js

@@ -6,20 +6,35 @@ const sinon = require('sinon');
 const fs = require('fs');
 const child_process = require('child_process');
 
+// Require the module once without clearing cache to avoid triggering
+// loadNativeAddon() repeatedly. The TestHelpers functions read process
+// state at call time, so they can be tested with platform/env changes
+// without re-requiring the module.
+// NOTE: The module may have been loaded by other tests before NODE_ENV was set,
+// so TestHelpers might not be present. We clear cache and re-require with
+// NODE_ENV='test' to ensure TestHelpers is exported.
+const nativeLoaderPath = require.resolve('../lib/native_loader.js');
+if (
+  !require.cache[nativeLoaderPath] ||
+  !require.cache[nativeLoaderPath].exports.TestHelpers
+) {
+  delete require.cache[nativeLoaderPath];
+  process.env.NODE_ENV = 'test';
+}
+const nativeLoader = require('../lib/native_loader.js');
+
 describe('NativeLoader testing', function () {
   const sandbox = sinon.createSandbox();
   let originalPlatform;
   let originalArch;
   let originalEnv;
+  let loader;
 
   beforeEach(function () {
     originalPlatform = process.platform;
     originalArch = process.arch;
     originalEnv = { ...process.env };
-    process.env.NODE_ENV = 'test';
-
-    // Clear cache to reload module
-    delete require.cache[require.resolve('../lib/native_loader.js')];
+    loader = nativeLoader.TestHelpers;
   });
 
   afterEach(function () {
@@ -29,20 +44,14 @@ describe('NativeLoader testing', function () {
     process.env = originalEnv;
   });
 
-  function getLoader() {
-    return require('../lib/native_loader.js').TestHelpers;
-  }
-
   it('customFallbackLoader returns null on non-linux', function () {
     Object.defineProperty(process, 'platform', { value: 'win32' });
-    const loader = getLoader();
     assert.strictEqual(loader.customFallbackLoader(), null);
   });
 
   it('customFallbackLoader returns null if env info missing', function () {
     Object.defineProperty(process, 'platform', { value: 'linux' });
     process.env.ROS_DISTRO = '';
-    const loader = getLoader();
     assert.strictEqual(loader.customFallbackLoader(), null);
   });
 
@@ -52,8 +61,6 @@ describe('NativeLoader testing', function () {
     process.env.ROS_DISTRO = 'humble';
 
     sandbox.stub(fs, 'existsSync').returns(false);
-
-    const loader = getLoader();
     assert.strictEqual(loader.customFallbackLoader(), null);
   });
 
@@ -64,8 +71,6 @@ describe('NativeLoader testing', function () {
 
     // Stub fs.existsSync to return true
     const existsSync = sandbox.stub(fs, 'existsSync').returns(true);
-
-    const loader = getLoader();
     assert.strictEqual(loader.customFallbackLoader(), null);
 
     // Verify it checked for the file
@@ -79,17 +84,12 @@ describe('NativeLoader testing', function () {
     process.env.RCLNODEJS_FORCE_BUILD = '1';
     const execSync = sandbox.stub(child_process, 'execSync');
 
-    // We expect it to try loading bindings after build
-    // Since we don't mock bindings, it will load the real one (if present) or fail.
-    // If it loads real one, test passes.
-    // If it fails, loadNativeAddon throws. We might need to handle that.
-    // But usually in dev env, the binding exists.
+    // Clear cache and re-require to trigger loadNativeAddon with RCLNODEJS_FORCE_BUILD.
+    // execSync is stubbed so no real rebuild (which deletes generated/) occurs.
+    delete require.cache[require.resolve('../lib/native_loader.js')];
 
     try {
-      getLoader();
-      // Wait, getLoader requires the file.
-      // The file calls loadNativeAddon() immediately.
-      // So valid test is just requiring the file.
+      require('../lib/native_loader.js');
     } catch (e) {
       // Ignore if loading binding fails, as long as execSync was called
     }

test/test-rosidl-message-generator.js

@@ -308,4 +308,46 @@ describe('ROSIDL Node.js message generator test suite', function () {
       process.env.AMENT_PREFIX_PATH = amentPrefixPathOriginal;
     }
   });
+
+  it('Testing mrpt_msgs/msg/GraphSlamAgents from non-standard msg subfolder', function () {
+    // GraphSlamAgents.msg lives under msg-common/ (non-standard subfolder name)
+    // and references GraphSlamAgent.msg from msg-ros2/. This verifies that
+    // packages with hyphenated subfolder names are generated and loadable.
+    const GraphSlamAgents = rclnodejs.require('mrpt_msgs/msg/GraphSlamAgents');
+    const GraphSlamAgent = rclnodejs.require('mrpt_msgs/msg/GraphSlamAgent');
+
+    // Verify GraphSlamAgents can be instantiated with an empty list
+    const agents = new GraphSlamAgents();
+    assert.ok(agents.list);
+    assert.equal(agents.list.size, 0);
+
+    // Verify GraphSlamAgent fields and defaults
+    const agent = new GraphSlamAgent();
+    assert.equal(agent.name.data, '');
+    assert.equal(agent.hostname.data, '');
+    assert.equal(agent.ip_addr.data, '');
+    assert.equal(agent.port, 0);
+    assert.equal(agent.is_online.data, false);
+    assert.equal(agent.last_seen_time.sec, 0);
+    assert.equal(agent.last_seen_time.nanosec, 0);
+    assert.equal(agent.topic_namespace.data, '');
+    assert.equal(agent.agent_id, 0);
+
+    // Verify field assignment
+    agent.name.data = 'robot_1';
+    agent.hostname.data = 'host1';
+    agent.ip_addr.data = '192.168.1.17';
+    agent.port = 11311;
+    agent.is_online.data = true;
+    agent.topic_namespace.data = 'robot_1';
+    agent.agent_id = 17;
+
+    assert.equal(agent.name.data, 'robot_1');
+    assert.equal(agent.hostname.data, 'host1');
+    assert.equal(agent.ip_addr.data, '192.168.1.17');
+    assert.equal(agent.port, 11311);
+    assert.equal(agent.is_online.data, true);
+    assert.equal(agent.topic_namespace.data, 'robot_1');
+    assert.equal(agent.agent_id, 17);
+  });
 });