chore(gax): resolve linter warnings in apiCallable unit tests - #9278
chore(gax): resolve linter warnings in apiCallable unit tests#9278shivanee-p wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors several unit tests in apiCallable.ts to use async/await syntax instead of callbacks and promise chains, improving readability and modernizing the test suite. The review feedback focuses on simplifying the assert.rejects assertions by passing the promises directly rather than wrapping them in redundant async functions, utilizing error constructors for type assertions, and removing unnecessary non-null assertion operators.
| await assert.rejects( | ||
| async () => { | ||
| await promise; | ||
| }, | ||
| (err: GoogleError) => { | ||
| assert(err instanceof GoogleError); | ||
| assert.strictEqual(err.code, status.CANCELLED); | ||
| done(); | ||
| }); | ||
| promise.cancel(); | ||
| return true; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Node's assert.rejects accepts a Promise directly as its first argument. Wrapping the promise in an async function is redundant and can be simplified.
await assert.rejects(
promise,
(err: GoogleError) => {
assert(err instanceof GoogleError);
assert.strictEqual(err.code, status.CANCELLED);
return true;
},
);| await assert.rejects(async () => { | ||
| await promise; | ||
| }); |
| await assert.rejects( | ||
| async () => { | ||
| await promise; | ||
| }, | ||
| (err: Error) => { | ||
| assert(err instanceof Error); | ||
| done(); | ||
| }); | ||
| return true; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
| await assert.rejects( | ||
| async () => { | ||
| await apiCall({}, undefined); | ||
| }, | ||
| (err: GoogleError) => { | ||
| assert(err instanceof Error); | ||
| assert.strictEqual(err!.code, FAKE_STATUS_CODE_1); | ||
| assert(err!.note); | ||
| const now = new Date(); | ||
| assert( | ||
| now.getTime() - startTime.getTime() >= backoff.totalTimeoutMillis!, | ||
| ); | ||
| const callsLowerBound = | ||
| backoff.totalTimeoutMillis! / | ||
| (backoff.maxRetryDelayMillis + backoff.maxRpcTimeoutMillis!); | ||
| const callsUpperBound = | ||
| backoff.totalTimeoutMillis! / backoff.initialRetryDelayMillis; | ||
| assert(spy.callCount > callsLowerBound); | ||
| assert(spy.callCount < callsUpperBound); | ||
| return true; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Node's assert.rejects accepts a Promise directly as its first argument. Additionally, since err is typed as GoogleError (which is non-nullable), the non-null assertion operator (!) is redundant and can be removed.
await assert.rejects(
apiCall({}, undefined),
(err: GoogleError) => {
assert(err instanceof Error);
assert.strictEqual(err.code, FAKE_STATUS_CODE_1);
assert(err.note);
const now = new Date();
assert(
now.getTime() - startTime.getTime() >= backoff.totalTimeoutMillis!,
);
const callsLowerBound =
backoff.totalTimeoutMillis! /
(backoff.maxRetryDelayMillis + backoff.maxRpcTimeoutMillis!);
const callsUpperBound =
backoff.totalTimeoutMillis! / backoff.initialRetryDelayMillis;
assert(spy.callCount > callsLowerBound);
assert(spy.callCount < callsUpperBound);
return true;
},
);eddcb84 to
23228f6
Compare
aaa69aa to
e2c6424
Compare
e2c6424 to
7d14d55
Compare
23228f6 to
e2b5e2e
Compare
Update Promises in apiCallable.ts to use modern async/await patterns