Skip to content

Commit 5614ed4

Browse files
ElshadHuEzraBrooks
ElshadHu
and
EzraBrooks
authored
Fix: Check status code in Action.sendGoal instead of result field (#1103)
Co-authored-by: Ezra Brooks <[email protected]>
1 parent da2f4ae commit 5614ed4

File tree

2 files changed

+349
-4
lines changed

2 files changed

+349
-4
lines changed

‎src/core/Action.ts‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,28 @@ import {
1414
import type Ros from "./Ros.ts";
1515
import { v4 as uuidv4 } from "uuid";
1616

17+
class GoalError extends Error {
18+
override name = "GoalError";
19+
constructor(status: GoalStatus, errorValue?: string) {
20+
super(`${makeErrorMessage(status)}${errorValue ? `: ${errorValue}` : ""}`);
21+
}
22+
}
23+
24+
function makeErrorMessage(status: GoalStatus) {
25+
switch (status) {
26+
case GoalStatus.STATUS_CANCELED:
27+
return `Action was canceled`;
28+
case GoalStatus.STATUS_ABORTED:
29+
return `Action was aborted`;
30+
case GoalStatus.STATUS_CANCELING:
31+
return `Action is canceling`;
32+
case GoalStatus.STATUS_UNKNOWN:
33+
return `Action status unknown`;
34+
default:
35+
return `Action failed with status ${String(status)}`;
36+
}
37+
}
38+
1739
/**
1840
* A ROS 2 action client.
1941
*/
@@ -69,19 +91,24 @@ export default class Action<
6991
}
7092

7193
const actionGoalId = `send_action_goal:${this.name}:${uuidv4()}`;
72-
73-
this.ros.on(actionGoalId, function (message) {
94+
this.ros.on(actionGoalId, (message) => {
7495
if (isRosbridgeActionResultMessage<TResult>(message)) {
96+
const status = message.status as GoalStatus;
97+
7598
if (!message.result) {
76-
failedCallback(message.values ?? "");
99+
failedCallback(String(new GoalError(status, message.values)));
100+
} else if (status !== GoalStatus.STATUS_SUCCEEDED) {
101+
failedCallback(
102+
String(new GoalError(status, JSON.stringify(message.values))),
103+
);
104+
// Check status code instead of result field to properly handle STATUS_CANCELED
77105
} else {
78106
resultCallback(message.values);
79107
}
80108
} else if (isRosbridgeActionFeedbackMessage<TFeedback>(message)) {
81109
feedbackCallback?.(message.values);
82110
}
83111
});
84-
85112
this.ros.callOnConnection({
86113
op: "send_action_goal",
87114
id: actionGoalId,

‎test/actionSendgoal.test.ts‎

Lines changed: 318 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,318 @@
1+
/**
2+
* @fileOverview
3+
* Test for Action.sendGoal status code handling
4+
* Tests that sendGoal properly handles STATUS_CANCELED and other status codes
5+
*/
6+
7+
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
8+
import Action from "../src/core/Action.ts";
9+
import { GoalStatus } from "../src/core/GoalStatus.ts";
10+
import type Ros from "../src/core/Ros.ts";
11+
12+
// Strict types matching the real protocol
13+
interface ActionResultMessageBase {
14+
op: "action_result";
15+
id: string;
16+
action: string;
17+
status: number;
18+
}
19+
20+
interface FailedActionResultMessage extends ActionResultMessageBase {
21+
result: false;
22+
values?: string;
23+
}
24+
25+
interface SuccessfulActionResultMessage extends ActionResultMessageBase {
26+
result: true;
27+
values: { result: number };
28+
}
29+
30+
type ActionResultMessage =
31+
| FailedActionResultMessage
32+
| SuccessfulActionResultMessage;
33+
34+
interface ActionFeedbackMessage {
35+
op: "action_feedback";
36+
id: string;
37+
action: string;
38+
values: { current: number };
39+
}
40+
41+
type ActionMessage = ActionResultMessage | ActionFeedbackMessage;
42+
43+
describe("Action.sendGoal", () => {
44+
let action: Action<
45+
{ target: number },
46+
{ current: number },
47+
{ result: number }
48+
>;
49+
let mockRos: Ros;
50+
let messageHandler: ((msg: ActionMessage) => void) | null = null;
51+
52+
beforeEach(() => {
53+
messageHandler = null;
54+
mockRos = {
55+
on: vi.fn((_id: string, callback: (msg: ActionMessage) => void) => {
56+
messageHandler = callback;
57+
}),
58+
callOnConnection: vi.fn(),
59+
} as unknown as Ros;
60+
61+
action = new Action({
62+
ros: mockRos,
63+
name: "/test_action",
64+
actionType: "test_msgs/TestAction",
65+
});
66+
});
67+
68+
afterEach(() => {
69+
vi.clearAllMocks();
70+
});
71+
72+
describe("status code handling", () => {
73+
it("should call resultCallback when action succeeds (STATUS_SUCCEEDED)", () => {
74+
const resultCallback = vi.fn();
75+
const failedCallback = vi.fn();
76+
77+
action.sendGoal(
78+
{ target: 10 },
79+
resultCallback,
80+
undefined,
81+
failedCallback,
82+
);
83+
84+
const successMessage: SuccessfulActionResultMessage = {
85+
op: "action_result",
86+
id: "test-id",
87+
action: "/test_action",
88+
values: { result: 42 },
89+
status: GoalStatus.STATUS_SUCCEEDED,
90+
result: true,
91+
};
92+
93+
messageHandler?.(successMessage);
94+
95+
expect(resultCallback).toHaveBeenCalledWith({ result: 42 });
96+
expect(failedCallback).not.toHaveBeenCalled();
97+
});
98+
99+
it("should call failedCallback when action is cancelled (STATUS_CANCELED)", () => {
100+
const resultCallback = vi.fn();
101+
const failedCallback = vi.fn();
102+
103+
action.sendGoal(
104+
{ target: 10 },
105+
resultCallback,
106+
undefined,
107+
failedCallback,
108+
);
109+
110+
const cancelledMessage: FailedActionResultMessage = {
111+
op: "action_result",
112+
id: "test-id",
113+
action: "/test_action",
114+
values: "Action was cancelled by user",
115+
status: GoalStatus.STATUS_CANCELED,
116+
result: false,
117+
};
118+
119+
messageHandler?.(cancelledMessage);
120+
121+
expect(failedCallback).toHaveBeenCalled();
122+
expect(resultCallback).not.toHaveBeenCalled();
123+
});
124+
125+
it("should call failedCallback when action is aborted (STATUS_ABORTED)", () => {
126+
const resultCallback = vi.fn();
127+
const failedCallback = vi.fn();
128+
129+
action.sendGoal(
130+
{ target: 10 },
131+
resultCallback,
132+
undefined,
133+
failedCallback,
134+
);
135+
136+
const abortedMessage: FailedActionResultMessage = {
137+
op: "action_result",
138+
id: "test-id",
139+
action: "/test_action",
140+
values: "Action aborted due to error",
141+
status: GoalStatus.STATUS_ABORTED,
142+
result: false,
143+
};
144+
145+
messageHandler?.(abortedMessage);
146+
147+
expect(failedCallback).toHaveBeenCalled();
148+
expect(resultCallback).not.toHaveBeenCalled();
149+
});
150+
151+
it("should call failedCallback when action is canceling (STATUS_CANCELING)", () => {
152+
const resultCallback = vi.fn();
153+
const failedCallback = vi.fn();
154+
155+
action.sendGoal(
156+
{ target: 10 },
157+
resultCallback,
158+
undefined,
159+
failedCallback,
160+
);
161+
162+
const cancelingMessage: FailedActionResultMessage = {
163+
op: "action_result",
164+
id: "test-id",
165+
action: "/test_action",
166+
values: "Action is being cancelled",
167+
status: GoalStatus.STATUS_CANCELING,
168+
result: false,
169+
};
170+
171+
messageHandler?.(cancelingMessage);
172+
173+
expect(failedCallback).toHaveBeenCalled();
174+
expect(resultCallback).not.toHaveBeenCalled();
175+
});
176+
177+
it("should handle unknown status gracefully (STATUS_UNKNOWN)", () => {
178+
const resultCallback = vi.fn();
179+
const failedCallback = vi.fn();
180+
181+
action.sendGoal(
182+
{ target: 10 },
183+
resultCallback,
184+
undefined,
185+
failedCallback,
186+
);
187+
188+
const unknownMessage: FailedActionResultMessage = {
189+
op: "action_result",
190+
id: "test-id",
191+
action: "/test_action",
192+
values: "Unknown status",
193+
status: GoalStatus.STATUS_UNKNOWN,
194+
result: false,
195+
};
196+
197+
messageHandler?.(unknownMessage);
198+
199+
expect(failedCallback).toHaveBeenCalled();
200+
expect(resultCallback).not.toHaveBeenCalled();
201+
});
202+
});
203+
204+
describe("feedback handling", () => {
205+
it("should handle feedback messages correctly", () => {
206+
const resultCallback = vi.fn();
207+
const feedbackCallback = vi.fn();
208+
const failedCallback = vi.fn();
209+
210+
action.sendGoal(
211+
{ target: 10 },
212+
resultCallback,
213+
feedbackCallback,
214+
failedCallback,
215+
);
216+
217+
const feedbackMessage: ActionFeedbackMessage = {
218+
op: "action_feedback",
219+
id: "test-id",
220+
action: "/test_action",
221+
values: { current: 5 },
222+
};
223+
224+
messageHandler?.(feedbackMessage);
225+
226+
expect(feedbackCallback).toHaveBeenCalledWith({ current: 5 });
227+
expect(resultCallback).not.toHaveBeenCalled();
228+
expect(failedCallback).not.toHaveBeenCalled();
229+
});
230+
231+
it("should handle multiple feedback messages", () => {
232+
const resultCallback = vi.fn();
233+
const feedbackCallback = vi.fn();
234+
const failedCallback = vi.fn();
235+
236+
action.sendGoal(
237+
{ target: 10 },
238+
resultCallback,
239+
feedbackCallback,
240+
failedCallback,
241+
);
242+
243+
const feedbackMessage1: ActionFeedbackMessage = {
244+
op: "action_feedback",
245+
id: "test-id",
246+
action: "/test_action",
247+
values: { current: 3 },
248+
};
249+
250+
const feedbackMessage2: ActionFeedbackMessage = {
251+
op: "action_feedback",
252+
id: "test-id",
253+
action: "/test_action",
254+
values: { current: 7 },
255+
};
256+
257+
messageHandler?.(feedbackMessage1);
258+
messageHandler?.(feedbackMessage2);
259+
260+
expect(feedbackCallback).toHaveBeenCalledTimes(2);
261+
expect(feedbackCallback).toHaveBeenNthCalledWith(1, { current: 3 });
262+
expect(feedbackCallback).toHaveBeenNthCalledWith(2, { current: 7 });
263+
});
264+
});
265+
266+
describe("edge cases", () => {
267+
it("should prioritize status code over result field", () => {
268+
const resultCallback = vi.fn();
269+
const failedCallback = vi.fn();
270+
271+
action.sendGoal(
272+
{ target: 10 },
273+
resultCallback,
274+
undefined,
275+
failedCallback,
276+
);
277+
278+
// This is the problematic case: result=true but status=CANCELED
279+
// According to protocol, this shouldn't happen, but we test it anyway
280+
// We have to cast to test this edge case since TypeScript prevents it
281+
const confusingMessage = {
282+
op: "action_result",
283+
id: "test-id",
284+
action: "/test_action",
285+
values: { result: 0 },
286+
status: GoalStatus.STATUS_CANCELED,
287+
result: true, // This violates the type but could happen at runtime
288+
} as ActionMessage;
289+
290+
messageHandler?.(confusingMessage);
291+
292+
// Should call failedCallback because status is CANCELED
293+
expect(failedCallback).toHaveBeenCalled();
294+
expect(resultCallback).not.toHaveBeenCalled();
295+
});
296+
297+
it("should handle missing feedback callback gracefully", () => {
298+
const resultCallback = vi.fn();
299+
const failedCallback = vi.fn();
300+
301+
action.sendGoal(
302+
{ target: 10 },
303+
resultCallback,
304+
undefined,
305+
failedCallback,
306+
);
307+
308+
const feedbackMessage: ActionFeedbackMessage = {
309+
op: "action_feedback",
310+
id: "test-id",
311+
action: "/test_action",
312+
values: { current: 5 },
313+
};
314+
315+
expect(() => messageHandler?.(feedbackMessage)).not.toThrow();
316+
});
317+
});
318+
});

0 commit comments

Comments
 (0)