Copilot commented on code in PR #3528:
URL: https://github.com/apache/thrift/pull/3528#discussion_r3295707462
##########
lib/nodets/test/test_driver.ts:
##########
@@ -149,7 +148,7 @@ export function ThriftTestDriverPromise(
.then(function (actual: any) {
assertionFn(actual, expected, fnName);
})
- .fail(fail("fnName"));
+ .catch(fail("fnName"));
Review Comment:
This `.catch` uses the literal string "fnName" instead of the actual
function name variable, so failures won’t report which RPC failed.
##########
compiler/cpp/src/thrift/generate/t_js_generator.cc:
##########
@@ -1570,7 +1590,7 @@ void t_js_generator::generate_process_function(t_service*
tservice, t_function*
<< ".length === " << fields.size() << ") {" << '\n';
indent_up();
- if (gen_es6_) {
+ if (gen_es6_ || gen_native_promise_) {
indent(f_service_) << "new Promise((resolve) => resolve(this._handler." <<
tfunction->get_name() << ".bind(this._handler)(" << '\n';
} else {
Review Comment:
The non-ES6 generator path (gen_es6_ == false) currently emits an arrow
function inside the Promise constructor (`(resolve) => ...`). That’s ES6 syntax
and breaks the intent of the non-ES6/ES5 output mode. If native Promises are
enabled in non-ES6 mode, the emitted JS should use `function(resolve) { ... }`
and avoid relying on arrow-function lexical `this` (capture `this` into a
`self` variable or bind explicitly).
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
Review Comment:
These `.then` branches call `fail(...)` but never invoke the returned
function, so the test will *not* fail if `testException(...)` unexpectedly
resolves. Use `assert.fail(...)` directly in the success path.
##########
lib/nodets/test/test_driver.ts:
##########
@@ -161,58 +160,58 @@ export function ThriftTestDriverPromise(
);
testCases.deep.forEach(makeAsserter(assert.deepEqual));
- Q.resolve(client.testStruct(testCases.out))
+ Promise.resolve(client.testStruct(testCases.out))
.then(function (response) {
checkRecursively(testCases.out, response, "testStruct");
})
- .fail(fail("testStruct"));
+ .catch(fail("testStruct"));
- Q.resolve(client.testNest(testCases.out2))
+ Promise.resolve(client.testNest(testCases.out2))
.then(function (response) {
checkRecursively(testCases.out2, response, "testNest");
})
- .fail(fail("testNest"));
+ .catch(fail("testNest"));
- Q.resolve(client.testInsanity(testCases.crazy))
+ Promise.resolve(client.testInsanity(testCases.crazy))
.then(function (response) {
checkRecursively(testCases.insanity, response, "testInsanity");
})
- .fail(fail("testInsanity"));
+ .catch(fail("testInsanity"));
- Q.resolve(client.testException("TException"))
+ Promise.resolve(client.testException("TException"))
.then(function (response) {
fail("testException: TException");
})
- .fail(function (err) {
+ .catch(function (err) {
assert.ok(err instanceof TException);
});
- Q.resolve(client.testException("Xception"))
+ Promise.resolve(client.testException("Xception"))
.then(function (response) {
fail("testException: Xception");
})
Review Comment:
These `.then` branches call `fail(...)` but never invoke the returned
function, so the test will *not* fail if `testException(...)` unexpectedly
resolves. Use `assert.fail(...)` directly in the success path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
