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.

Sign up for GitHub

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

Jump to bottom

[OTE-881] propagate bazooka failure in auxo #2537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[OTE-881] propagate bazooka failure in auxo #2537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 72 additions & 3 deletions indexer/services/auxo/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,74 @@
describe('index', () => {
it('true is true', () => {
expect(true).toEqual(true);
// handler.test.ts
import { handler } from '../src/index';
import * as helpers from '../src/helpers';
import { InvokeCommandOutput, LambdaClient } from '@aws-sdk/client-lambda';
import { APIGatewayEvent, Context } from 'aws-lambda';
import { AuxoEventJson } from 'src/types';

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix the import path for types.

The import path 'src/types' appears to be absolute. It should be relative to maintain consistency and avoid potential module resolution issues.

-import { AuxoEventJson } from 'src/types';
+import { AuxoEventJson } from '../src/types';

Committable suggestion was skipped due to low confidence.

// Mock logger and startBugsnag from @dydxprotocol-indexer/base
jest.mock('@dydxprotocol-indexer/base', () => {
const originalModule = jest.requireActual('@dydxprotocol-indexer/base');
return {
...originalModule,
logger: {
info: jest.fn(),
error: jest.fn(),
crit: jest.fn(),
},
startBugsnag: jest.fn(),
};
});

describe('Auxo Handler', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should return 500 when Bazooka Lambda errors', async () => {
// Mock upgradeBazooka to do nothing
jest.spyOn(helpers, 'upgradeBazooka').mockResolvedValue(undefined);

// Mock LambdaClient.send to return response with FunctionError
jest.spyOn(LambdaClient.prototype, 'send').mockImplementation(() => {
return {
StatusCode: 500,
FunctionError: 'Some bazooka error',
$metadata: {
httpStatusCode: 200, // api returns 200 even if lambda runtime error
requestId: 'mock-request-id-invoke',
extendedRequestId: 'mock-extended-request-id-invoke',
cfId: 'mock-cf-id-invoke',
attempts: 1,
totalRetryDelay: 0,
},
} as InvokeCommandOutput;
});

const mockEvent: APIGatewayEvent & AuxoEventJson = {
// APIGatewayEvent properties
body: null,
headers: {},
multiValueHeaders: {},
httpMethod: 'POST',
isBase64Encoded: false,
path: '/deploy',
pathParameters: null,
queryStringParameters: null,
multiValueQueryStringParameters: null,
stageVariables: null,
resource: '',
requestContext: {} as any,
// AuxoEventJson properties
upgrade_tag: 'some_tag',
prefix: 'some_prefix',
region: 'us-east-1',
regionAbbrev: 'us-east-1',
addNewKafkaTopics: false,
onlyRunDbMigrationAndCreateKafkaTopics: false,
};

const mockContext: Context = {} as any;

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve mock context implementation.

The current mock context is too minimal ({} as any), which could hide potential issues. Implement a proper mock context with all required properties.

-const mockContext: Context = {} as any;
+const mockContext: Context = {
+  awsRequestId: 'mock-request-id',
+  functionName: 'auxo-handler',
+  functionVersion: '1',
+  invokedFunctionArn: 'arn:aws:lambda:us-east-1:123456789012:function:auxo-handler',
+  memoryLimitInMB: '128',
+  logGroupName: '/aws/lambda/auxo-handler',
+  logStreamName: '2024/01/01/[$LATEST]mock',
+  callbackWaitsForEmptyEventLoop: true,
+  getRemainingTimeInMillis: () => 30000,
+  done: jest.fn(),
+  fail: jest.fn(),
+  succeed: jest.fn(),
+};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const mockContext: Context = {} as any;
const mockContext: Context = {
awsRequestId: 'mock-request-id',
functionName: 'auxo-handler',
functionVersion: '1',
invokedFunctionArn: 'arn:aws:lambda:us-east-1:123456789012:function:auxo-handler',
memoryLimitInMB: '128',
logGroupName: '/aws/lambda/auxo-handler',
logStreamName: '2024/01/01/[$LATEST]mock',
callbackWaitsForEmptyEventLoop: true,
getRemainingTimeInMillis: () => 30000,
done: jest.fn(),
fail: jest.fn(),
succeed: jest.fn(),
};

await expect(handler(mockEvent, mockContext)).rejects.toThrow('bazooka failure: Some bazooka error');
});
Comment on lines +27 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add comprehensive test coverage.

The current test file only covers the Bazooka Lambda error case. Based on the PR objectives and changes, we need additional test coverage for:

  1. Success scenarios:

    • Successful deployment without Kafka topics
    • Successful deployment with new Kafka topics
    • Successful database migration only

  2. Error scenarios:

    • Database migration failures
    • ECS service upgrade failures
    • Network/timeout errors

  3. Edge cases:

    • Invalid event properties
    • Missing required fields
    • Unexpected Lambda responses

Here's a template for additional test cases:

// Success scenarios
it('should successfully complete deployment without Kafka topics', async () => {
  // Mock successful responses
  jest.spyOn(helpers, 'upgradeBazooka').mockResolvedValue(undefined);
  jest.spyOn(helpers, 'runDbAndKafkaMigration').mockResolvedValue(undefined);
  
  const mockEvent = {
    ...baseMockEvent,
    addNewKafkaTopics: false,
  };
  
  await expect(handler(mockEvent, mockContext)).resolves.not.toThrow();
  expect(helpers.upgradeBazooka).toHaveBeenCalledTimes(1);
  expect(helpers.runDbAndKafkaMigration).toHaveBeenCalledWith(
    expect.objectContaining({ createNewKafkaTopics: false })
  );
});

it('should only run database migration when onlyRunDbMigrationAndCreateKafkaTopics is true', async () => {
  // ... similar structure for other cases
});

});
Loading
Loading