Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use builtin library and add test to verify that delay is actually delaying #613

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use builtin library and add test to verify that delay is actually delaying #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a60938
Use builtin library and add test to verify that delay is actually del…
phillipperalez Jul 20, 2024
e3b0cf8
Lint errors
phillipperalez Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/actions/google/drive/sheets/google_sheets.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ export declare class GoogleSheetsAction extends GoogleDriveAction {
retriableFileList(drive: Drive, options: any, attempt: number, webhookId: string): Promise<any>;
flush(buffer: sheets_v4.Schema$BatchUpdateSpreadsheetRequest, sheet: Sheet, spreadsheetId: string, webhookId: string): Promise<import("gaxios").GaxiosResponse<sheets_v4.Schema$BatchUpdateSpreadsheetResponse>>;
flushRetry(buffer: sheets_v4.Schema$BatchUpdateSpreadsheetRequest, sheet: Sheet, spreadsheetId: string, webhookId: string): Promise<import("gaxios").GaxiosResponse<sheets_v4.Schema$BatchUpdateSpreadsheetResponse>>;
protected delay(time: number): Promise<void>;
delay(time: number): Promise<void>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What effect are you achieving here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't particularly matter and then I can actually test the function

protected sheetsClientFromRequest(redirect: string, tokens: Credentials): Promise<sheets_v4.Sheets>;
}
9 changes: 4 additions & 5 deletions lib/actions/google/drive/sheets/google_sheets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ const http_errors_1 = require("../../../../error_types/http_errors");
const Hub = require("../../../../hub");
const parse = require("csv-parse");
const googleapis_1 = require("googleapis");
const promises_1 = require("timers/promises");
const winston = require("winston");
const utils_1 = require("../../../../error_types/utils");
const action_response_1 = require("../../../../hub/action_response");
const hub_1 = require("../../../../hub");
const google_drive_1 = require("../google_drive");
const MAX_REQUEST_BATCH = process.env.GOOGLE_SHEETS_WRITE_BATCH ? Number(process.env.GOOGLE_SHEETS_WRITE_BATCH) : 4096;
const MAX_ROW_BUFFER_INCREASE = 6000;
Expand Down Expand Up @@ -75,7 +76,7 @@ class GoogleSheetsAction extends google_drive_1.GoogleDriveAction {
catch (e) {
this.sanitizeGaxiosError(e);
const errorType = (0, utils_1.getHttpErrorType)(e, this.name);
let error = (0, action_response_1.errorWith)(errorType, `${LOG_PREFIX} ${e.toString()}`);
let error = (0, hub_1.errorWith)(errorType, `${LOG_PREFIX} ${e.toString()}`);
if (e.code && e.errors && e.errors[0] && e.errors[0].message) {
error = Object.assign(Object.assign({}, error), { http_code: e.code, message: `${errorType.description} ${LOG_PREFIX} ${e.errors[0].message}` });
}
Expand Down Expand Up @@ -446,9 +447,7 @@ class GoogleSheetsAction extends google_drive_1.GoogleDriveAction {
}
delay(time) {
return __awaiter(this, void 0, void 0, function* () {
yield new Promise((resolve) => {
setTimeout(resolve, time);
});
return yield (0, promises_1.setTimeout)(time).catch((_) => { winston.error("setTimeout throw!"); });
});
}
sheetsClientFromRequest(redirect, tokens) {
Expand Down
9 changes: 4 additions & 5 deletions src/actions/google/drive/sheets/google_sheets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import * as parse from "csv-parse"
import { Credentials } from "google-auth-library"
import { drive_v3, google, sheets_v4 } from "googleapis"
import { GaxiosPromise } from "googleapis-common"
import {setTimeout} from "timers/promises"
import * as winston from "winston"
import { getHttpErrorType } from "../../../../error_types/utils"
import { Error, errorWith } from "../../../../hub/action_response"
import { Error, errorWith } from "../../../../hub"
import { GoogleDriveAction } from "../google_drive"
import Drive = drive_v3.Drive
import Sheet = sheets_v4.Sheets
Expand Down Expand Up @@ -444,10 +445,8 @@ export class GoogleSheetsAction extends GoogleDriveAction {
throw `Max retries attempted`
}

protected async delay(time: number) {
await new Promise<void>((resolve) => {
setTimeout(resolve, time)
})
async delay(time: number) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the units here?

return await setTimeout(time).catch((_) => { winston.error("setTimeout throw!")})
}

protected async sheetsClientFromRequest(redirect: string, tokens: Credentials) {
Expand Down
9 changes: 9 additions & 0 deletions src/actions/google/drive/sheets/test_google_sheets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,15 @@ describe(`${action.constructor.name} unit tests`, () => {
})
})

describe("delay", () => {
it("will block on await delay", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you provide more detail for why this is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the previous implementation, although it looked correct, was not actually delaying retries. I am seeing another area which I missed so I will upload another change to this as well

const start = Date.now()
await action.delay(100)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the units here?

const end = Date.now()
chai.expect(end - start).to.be.greaterThanOrEqual(10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the units here?

})
})

describe("flushRetry", () => {
it("will retry until the MAX_RETRY_LIMIT is reached", (done) => {
const delayStub = sinon.stub(action as any, "delay")
Expand Down
Loading