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

add code to record and play back tests from real data #2307

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

johnduffell
Copy link
Member

@johnduffell johnduffell commented Jun 5, 2024

This is a 10% time PR so although I'm convinced it's a good idea, there may need to be more thought applied.

At the moment we always try our best to write tests manually, based on all the typical and error cases we can think of.
Unfortunately sometimes we miss adding tests when we add new products or codepaths, even if they are very common ones. Or the tests have a gap in them meaning they don't truly test end to end.

This PR tries to address that by recording 2 weeks worth of real requests and responses into S3, and letting us regression test the API e.g. when we bump libraries or refactor.

Test data is recorded here for CODE (and correspondingly for DEV):
https://eu-west-1.console.aws.amazon.com/s3/buckets/gu-reader-revenue-logs?prefix=CODE/discount-api/&region=eu-west-1&bucketType=general

There are a couple of issues where the code is non determinstic.
First is this piece of code where the first rejected promise cancels the other and fails the overall promise. This means sometimes we don't have a billing preview in the store, so then that can fail first when rerunning the test. Not sure how to handle that off hand

const [, billingPreview] = await Promise.all([
checkSubscriptionBelongsToIdentityId(zuoraClient, subscription, identityId),
getBillingPreview(
zuoraClient,
dayjs().add(13, 'months'),
subscription.accountNumber,
),
]);

Also there is the date used in a few places, so the test might fail if we move on to the next month between recording the rerunning the tests. I think that isn't an issue that needs to be fixed in this PR but we would need record and pass the time in rather than getting it during the code execution.


const stage = process.env.STAGE as Stage;
export const handler: Handler = async (
export const handler: (
event: APIGatewayProxyEvent,
Copy link
Member Author

Choose a reason for hiding this comment

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

We only need the event, not the other values e.g. context, but when I call the handler explicitly from the test it requires those other values.
Rather than magicking them up, I just changed the type.

Comment on lines 14 to 16
const requestLogger = new RequestLogger(stage || 'DEV');
requestLogger.setRequest(JSON.stringify(event));
Copy link
Member Author

Choose a reason for hiding this comment

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

the request logger gets a copy of the incoming event to store, and if it's running locally it places them in DEV key in the bucket.

console.log(`Response is ${JSON.stringify(response)}`);
await requestLogger.setResponse(response);
Copy link
Member Author

Choose a reason for hiding this comment

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

the request logger also gets a copy of the response to store, and when it gets that it flushes everything out to S3

try {
switch (true) {
case event.path === '/apply-discount' && event.httpMethod === 'POST': {
console.log('Applying a discount');
return await discountEndpoint(stage, false, event.headers, event.body);
return await discountEndpoint(
stage || 'CODE',
Copy link
Member Author

Choose a reason for hiding this comment

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

if we are running in DEV, it uses CODE (i.e. sandbox) zuora.

Comment on lines +16 to +22
test('call the lambda with all recorded data', async () => {
const keys = await getKeys('DEV');
for (const key of keys) {
const requestPlayback = await RequestPlayback.loadKey(key);
await testSingle(requestPlayback);
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

this test iterates through all the saved values in S3 (for DEV) and runs tests against them all. e.g.
https://eu-west-1.console.aws.amazon.com/s3/buckets/gu-reader-revenue-logs?prefix=DEV/discount-api/&region=eu-west-1&bucketType=general

Comment on lines 24 to 28
test('call the lambda with recorded data', async () => {
// load data from s3
const requestPlayback = await RequestPlayback.load('DEV', '1717622177150');
await testSingle(requestPlayback);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

this test just runs the one stored data execution by its timestamp (as a number)

Comment on lines 30 to 44
test('call the lambda to record data - preview', async () => {
const testDataObject = JSON.parse(previewTestData);
// const expected = '';
const response = await handler(testDataObject);
expect(response.statusCode).toEqual(200);
expect(response.body).toEqual(expectedBody);
}, 30000);

test('call the lambda to record data - apply', async () => {
const testDataObject = JSON.parse(applyTestData);
// const expected = '';
const response = await handler(testDataObject);
expect(response.statusCode).toEqual(200);
expect(response.body).toEqual('Success');
}, 30000);
Copy link
Member Author

Choose a reason for hiding this comment

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

these plus the data below were useful for running locally to record some data to S3.
They should probably be ignored or written as a proper integration test that creates its own subscription and cancels it afterwards.

@@ -10,6 +10,6 @@
},
"devDependencies": {
"@types/aws-lambda": "^8.10.129",
"@aws-sdk/client-s3": "3.451.0"
"@aws-sdk/client-s3": "3.590.0"
Copy link
Member Author

@johnduffell johnduffell Jun 5, 2024

Choose a reason for hiding this comment

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

I bumped this everywhere, as I was getting a strange error, but maybe that was unrelated.
endpointFunctions_1.endpointFunctions[fn] is not a function

method: 'POST',
headers: {
'Content-type': 'application/x-www-form-urlencoded',
Copy link
Member Author

Choose a reason for hiding this comment

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

this header needs to be explicitly set - I think if you pass in URLSearchParams it does it automatically.

@@ -40,14 +42,17 @@ export class BearerTokenProvider {
['client_id', this.credentials.clientId],
['client_secret', this.credentials.clientSecret],
['grant_type', 'client_credentials'],
]);
]).toString();
Copy link
Member Author

Choose a reason for hiding this comment

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

URLSearchParams doesn't serialise to anything when I try to write it to S3 so I have to make it a json type (i.e. string)

body: formData,
});

const json = await response.json();
const json = JSON.parse(response.text);
Copy link
Member Author

Choose a reason for hiding this comment

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

it's hard to preserve the types if I try to keep it as json when writing/reading it to the file in s3, so I just store the string and make peolpe parse it when they get it out. It might be possible to add a helper extension method but I didn't know how off hand.

this.request = request;
};

setResponse = async (response: ResponseData) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

response might be better as a string? - for API gateway (this lambda) we always have a statusCode and a body, but that wouldn't be the case if we extend this to SQS or time triggered lambdas later.

);
console.log(`Response is ${response.statusCode}\n${response.body}`);
expect(response.statusCode).toEqual(requestPlayback.response.statusCode);
expect(response.body).toEqual(requestPlayback.response.body);
Copy link
Member Author

Choose a reason for hiding this comment

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

it's a bit tricky to compare the body, as we don't care about the order of the keys. At the moment it seems to work ok, but it might have to get cleverer if a refactor changes the order of things around.

console.log('Response from Zuora was: ', JSON.stringify(json, null, 2));

if (response.ok) {
return schema.parse(json);
} else {
console.error(response.text);
Copy link
Member Author

Choose a reason for hiding this comment

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

this would just crash it, as if you already did response.json, then response.text fails with an error.

@johnduffell johnduffell force-pushed the jd-record-requests branch 3 times, most recently from 69045fb to fad4d58 Compare June 5, 2024 22:23
Base automatically changed from jd-supporterplus-free-save to main June 10, 2024 09:18
# Conflicts:
#	handlers/discount-api/src/discountEndpoint.ts
#	handlers/discount-api/src/index.ts
#	handlers/discount-api/test/previewDiscountIntegration.test.ts
#	handlers/product-switch-api/package.json
#	modules/email/package.json
#	modules/zuora/src/bearerTokenProvider.ts
#	pnpm-lock.yaml
# Conflicts:
#	handlers/discount-api/package.json
#	handlers/generate-product-catalog/package.json
#	modules/zuora-catalog/package.json
#	modules/zuora/package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant