Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Prevent flakiness #52

Merged
merged 19 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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 .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:

strategy:
matrix:
node-version: [18.x, 20.x]
node-version: [18.x, 20.x, 21.x]

steps:
- uses: actions/checkout@v4
Expand Down
3 changes: 1 addition & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ services:
- rabbit_data:/var/lib/rabbitmq

localstack:
image: localstack/localstack:2.3.2
image: localstack/localstack:3.0.0
network_mode: bridge
hostname: localstack
ports:
Expand All @@ -19,7 +19,6 @@ services:
- DEBUG=0
- DATA_DIR=${DATA_DIR-}
- DOCKER_HOST=unix:///var/run/docker.sock
- HOSTNAME_EXTERNAL=localstack
- LOCALSTACK_HOST=localstack
# - LOCALSTACK_API_KEY=someDummyKey
volumes:
Expand Down
27 changes: 14 additions & 13 deletions packages/amqp/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"prepublishOnly": "npm run build:release"
},
"dependencies": {
"@lokalise/node-core": "^7.1.0",
"@lokalise/node-core": "^8.1.3",
"zod": "^3.22.4"
},
"peerDependencies": {
Expand All @@ -35,23 +35,24 @@
},
"devDependencies": {
"@message-queue-toolkit/core": "*",
"@types/amqplib": "^0.10.1",
"@types/node": "^20.7.0",
"@typescript-eslint/eslint-plugin": "^6.7.3",
"@typescript-eslint/parser": "^6.7.3",
"@vitest/coverage-v8": "^0.34.5",
"@types/amqplib": "^0.10.4",
"@types/node": "^20.9.2",
"@typescript-eslint/eslint-plugin": "^6.11.0",
"@typescript-eslint/parser": "^6.11.0",
"@vitest/coverage-v8": "^0.34.6",
"amqplib": "^0.10.3",
"awilix": "^9.0.0",
"awilix-manager": "^4.0.0",
"del-cli": "^5.0.0",
"eslint": "^8.50.0",
"del-cli": "^5.1.0",
"eslint": "^8.54.0",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-prettier": "^5.0.0",
"eslint-plugin-vitest": "^0.3.1",
"prettier": "^3.0.0",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-prettier": "^5.0.1",
"eslint-plugin-vitest": "^0.3.10",
"prettier": "^3.1.0",
"typescript": "^5.2.2",
"vitest": "^0.34.5"
"vitest": "^0.34.6",
"vite": "4.5.0"
},
"homepage": "https://github.com/kibertoad/message-queue-toolkit",
"repository": {
Expand Down
14 changes: 11 additions & 3 deletions packages/core/lib/queues/AbstractQueueService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export type ExistingQueueOptionsMultiSchema<

export type DeletionConfig = {
deleteIfExists?: boolean
waitForConfirmation?: boolean
forceDeleteInProduction?: boolean
}

Expand Down Expand Up @@ -132,11 +133,18 @@ export abstract class AbstractQueueService<
this.logger.debug(messageLogEntry)
}

protected handleError(err: unknown) {
protected handleError(err: unknown, context?: Record<string, unknown>) {
const logObject = resolveGlobalErrorLogObject(err)
this.logger.error(logObject)
if (logObject === 'string') {
this.logger.error(context, logObject)
} else if (typeof logObject === 'object') {
this.logger.error({
...logObject,
...context,
})
}
if (types.isNativeError(err)) {
this.errorReporter.report({ error: err })
this.errorReporter.report({ error: err, context })
}
}

Expand Down
16 changes: 8 additions & 8 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@
},
"dependencies": {},
"devDependencies": {
"@types/node": "^20.7.0",
"@typescript-eslint/eslint-plugin": "^6.7.3",
"@typescript-eslint/parser": "^6.7.3",
"del-cli": "^5.0.0",
"eslint": "^8.44.0",
"@types/node": "^20.9.2",
"@typescript-eslint/eslint-plugin": "^6.11.0",
"@typescript-eslint/parser": "^6.11.0",
"del-cli": "^5.1.0",
"eslint": "^8.54.0",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-prettier": "^5.0.0",
"prettier": "^3.0.0",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-prettier": "^5.0.1",
"prettier": "^3.1.0",
"typescript": "^5.2.2"
},
"homepage": "https://github.com/kibertoad/message-queue-toolkit",
Expand Down
2 changes: 1 addition & 1 deletion packages/core/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"sourceMap": true,
"declaration": true,
"declarationMap": false,
"types": ["node", "vitest/globals"],
"types": ["node"],
"strict": true,
"moduleResolution": "node",
"noUnusedLocals": false,
Expand Down
8 changes: 4 additions & 4 deletions packages/sns/.eslintignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
node_modules/
coverage/
dist/
vitest.config.ts
node_modules/
coverage/
dist/
vitest.config.ts
3 changes: 1 addition & 2 deletions packages/sns/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
services:
localstack:
image: localstack/localstack:2.3.2
image: localstack/localstack:3.0.0
network_mode: bridge
hostname: localstack
ports:
Expand All @@ -11,7 +11,6 @@ services:
- DEBUG=0
- DATA_DIR=${DATA_DIR-}
- DOCKER_HOST=unix:///var/run/docker.sock
- HOSTNAME_EXTERNAL=localstack
- LOCALSTACK_HOST=localstack
# - LOCALSTACK_API_KEY=someDummyKey
volumes:
Expand Down
8 changes: 6 additions & 2 deletions packages/sns/lib/utils/snsInitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,12 @@ export async function deleteSnsSqs(
throw new Error('subscriptionArn must be set for automatic deletion')
}

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await deleteQueue(sqsClient, queueConfiguration.QueueName!)
await deleteQueue(
sqsClient,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
queueConfiguration.QueueName!,
deletionConfig.waitForConfirmation !== false,
)
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await deleteTopic(snsClient, topicConfiguration.Name!)
await deleteSubscription(snsClient, subscriptionArn)
Expand Down
35 changes: 18 additions & 17 deletions packages/sns/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,36 +26,37 @@
"prepublishOnly": "npm run build:release"
},
"dependencies": {
"@lokalise/node-core": "^7.1.0",
"sqs-consumer": "^7.3.0",
"@lokalise/node-core": "^8.1.3",
"sqs-consumer": "^7.4.0",
"zod": "^3.22.4"
},
"peerDependencies": {
"@aws-sdk/client-sns": "^3.431.0",
"@aws-sdk/client-sqs": "^3.431.0",
"@aws-sdk/client-sns": "^3.454.0",
"@aws-sdk/client-sqs": "^3.454.0",
"@message-queue-toolkit/core": "^4.0.0",
"@message-queue-toolkit/sqs": "^5.0.0"
},
"devDependencies": {
"@aws-sdk/client-sns": "^3.431.0",
"@aws-sdk/client-sqs": "^3.431.0",
"@aws-sdk/client-sns": "^3.454.0",
"@aws-sdk/client-sqs": "^3.454.0",
"@message-queue-toolkit/core": "*",
"@message-queue-toolkit/sqs": "*",
"@types/node": "^20.7.0",
"@typescript-eslint/eslint-plugin": "^6.7.3",
"@typescript-eslint/parser": "^6.7.3",
"@vitest/coverage-v8": "^0.34.5",
"@types/node": "^20.9.2",
"@typescript-eslint/eslint-plugin": "^6.11.0",
"@typescript-eslint/parser": "^6.11.0",
"@vitest/coverage-v8": "^0.34.6",
"awilix": "^9.0.0",
"awilix-manager": "^4.0.0",
"del-cli": "^5.0.0",
"eslint": "^8.50.0",
"del-cli": "^5.1.0",
"eslint": "^8.54.0",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-prettier": "^5.0.0",
"eslint-plugin-vitest": "^0.3.1",
"prettier": "^3.0.0",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-prettier": "^5.0.1",
"eslint-plugin-vitest": "^0.3.10",
"prettier": "^3.1.0",
"typescript": "^5.2.2",
"vitest": "^0.34.5"
"vitest": "^0.34.6",
"vite": "4.5.0"
},
"homepage": "https://github.com/kibertoad/message-queue-toolkit",
"repository": {
Expand Down
6 changes: 2 additions & 4 deletions packages/sns/test/utils/testContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ export async function registerDependencies(
awilixManager: asFunction(() => {
return awilixManager
}, SINGLETON_CONFIG),

// Not disposing sqs client allows consumers to terminate correctly
sqsClient: asFunction(
() => {
return new SQSClient(TEST_AWS_CONFIG)
},
{
lifetime: Lifetime.SINGLETON,
asyncDispose: 'destroy',
asyncDisposePriority: 40,
},
),
snsClient: asFunction(
Expand All @@ -58,8 +58,6 @@ export async function registerDependencies(
},
{
lifetime: Lifetime.SINGLETON,
asyncDispose: 'destroy',
asyncDisposePriority: 40,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm, really interesting and good to know! It might be causing flakiness problems in library clients, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah, I would expect so. we should also do this change on autopilot and check if it also works fine there

},
),

Expand Down
3 changes: 1 addition & 2 deletions packages/sqs/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
services:
localstack:
image: localstack/localstack:2.3.2
image: localstack/localstack:3.0.0
network_mode: bridge
hostname: localstack
ports:
Expand All @@ -11,7 +11,6 @@ services:
- DEBUG=0
- DATA_DIR=${DATA_DIR-}
- DOCKER_HOST=unix:///var/run/docker.sock
- HOSTNAME_EXTERNAL=localstack
- LOCALSTACK_HOST=localstack
# - LOCALSTACK_API_KEY=someDummyKey
volumes:
Expand Down
4 changes: 3 additions & 1 deletion packages/sqs/lib/sqs/AbstractSqsConsumer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,9 @@ export abstract class AbstractSqsConsumer<
})

this.consumer.on('error', (err) => {
this.handleError(err)
this.handleError(err, {
queueName: this.queueName,
})
})

this.consumer.start()
Expand Down
6 changes: 5 additions & 1 deletion packages/sqs/lib/utils/sqsInitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ export async function deleteSqs(
throw new Error('QueueName must be set for automatic deletion')
}

await deleteQueue(sqsClient, creationConfig.queue.QueueName)
await deleteQueue(
sqsClient,
creationConfig.queue.QueueName,
deletionConfig.waitForConfirmation !== false,
)
}

export async function initSqs(
Expand Down
25 changes: 24 additions & 1 deletion packages/sqs/lib/utils/sqsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import {
DeleteQueueCommand,
GetQueueAttributesCommand,
SetQueueAttributesCommand,
ListQueuesCommand,
} from '@aws-sdk/client-sqs'
import type { QueueAttributeName } from '@aws-sdk/client-sqs/dist-types/models/models_0'
import type { Either } from '@lokalise/node-core'
import { waitAndRetry } from '@message-queue-toolkit/core'

import type { ExtraSQSCreationParams } from '../sqs/AbstractSqsConsumer'
import type { SQSQueueLocatorType } from '../sqs/AbstractSqsService'
Expand Down Expand Up @@ -94,7 +96,11 @@ export async function assertQueue(
}
}

export async function deleteQueue(client: SQSClient, queueName: string) {
export async function deleteQueue(
client: SQSClient,
queueName: string,
waitForConfirmation = true,
) {
try {
const queueUrlCommand = new GetQueueUrlCommand({
QueueName: queueName,
Expand All @@ -106,7 +112,24 @@ export async function deleteQueue(client: SQSClient, queueName: string) {
})

await client.send(command)

if (waitForConfirmation) {
await waitAndRetry(async () => {
const queueList = await client.send(
new ListQueuesCommand({
QueueNamePrefix: queueName,
}),
)
return !queueList.QueueUrls || queueList.QueueUrls.length === 0
})
}
} catch (err) {
// This is fine
// @ts-ignore
if (err.name === 'QueueDoesNotExist') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🟢 Should this be a constant?

return
}

// @ts-ignore
console.log(`Failed to delete: ${err.message}`)
}
Expand Down
31 changes: 16 additions & 15 deletions packages/sqs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,33 @@
"prepublishOnly": "npm run build:release"
},
"dependencies": {
"@lokalise/node-core": "^7.1.0",
"sqs-consumer": "^7.3.0",
"@lokalise/node-core": "^8.1.3",
"sqs-consumer": "^7.4.0",
"zod": "^3.22.4"
},
"peerDependencies": {
"@aws-sdk/client-sqs": "^3.431.0",
"@aws-sdk/client-sqs": "^3.454.0",
"@message-queue-toolkit/core": "^4.0.0"
},
"devDependencies": {
"@aws-sdk/client-sqs": "^3.431.0",
"@aws-sdk/client-sqs": "^3.454.0",
"@message-queue-toolkit/core": "*",
"@types/node": "^20.7.0",
"@typescript-eslint/eslint-plugin": "^6.7.3",
"@typescript-eslint/parser": "^6.7.3",
"@vitest/coverage-v8": "^0.34.5",
"@types/node": "^20.9.2",
"@typescript-eslint/eslint-plugin": "^6.11.0",
"@typescript-eslint/parser": "^6.11.0",
"@vitest/coverage-v8": "^0.34.6",
"awilix": "^9.0.0",
"awilix-manager": "^4.0.0",
"del-cli": "^5.0.0",
"eslint": "^8.47.0",
"del-cli": "^5.1.0",
"eslint": "^8.54.0",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-prettier": "^5.0.0",
"eslint-plugin-vitest": "^0.3.2",
"prettier": "^3.0.0",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-prettier": "^5.0.1",
"eslint-plugin-vitest": "^0.3.10",
"prettier": "^3.1.0",
"typescript": "^5.2.2",
"vitest": "^0.34.5"
"vitest": "^0.34.6",
"vite": "4.5.0"
},
"homepage": "https://github.com/kibertoad/message-queue-toolkit",
"repository": {
Expand Down
Loading
Loading