kwicherbelliaken / bad-reviews-make-good-movies

0 stars 0 forks source link

[TEST]: add movie to watchlist and delete movie from watchlist endpoints #85

Closed slackermorris closed 5 months ago

slackermorris commented 6 months ago

These feel like good reads:

  1. Mocking AWS SDK V3: https://blog.chriscatt.com/jest-mocking-with-aws-sdk-v3
  2. Mocking moduler AWS SDK: https://aws.amazon.com/blogs/developer/mocking-modular-aws-sdk-for-javascript-v3-in-unit-tests/
  3. https://jeffsegovia.dev/blogs/rest-api-validation-using-zod
  4. Verify HTTP response bodies: https://timdeschryver.dev/blog/why-we-should-verify-http-response-bodies-and-why-we-should-use-zod-for-this#result
slackermorris commented 6 months ago

For the add endpoint I am trying to figure out how to generate a zod schema from a typescript type.

slackermorris commented 6 months ago

I don't like how much information we are passing to the add endpoint: username, watchlistId... username seems redundant. Surely we can extract this from the user session in the middleware using clerk or something of the kind.

watchlistId can likely be passed as part of the path parameters.

Whelp. Do I add Clerk?

slackermorris commented 6 months ago

Here's the indication that I should test the handler independent of the middy wrapper.

Screen Shot 2024-01-06 at 9 24 35 AM
slackermorris commented 6 months ago

So, I have some difficulty.

The handlerWrapper initially wrapped the lambdas and processed errors that might occur.

I then introduced middy middleware. This meant that I was testing both the middy middleware as well as the body logic of the handler (including error handling as contributed by the wrapper). Shit. I can probably do the error handling using a middy middleware tbh.

A difficulty arises when you are testing the handler because the middy middleware (JSON body parser) expects the event to be a legitimate http request (body is stringified). Which would be a PITA to have to recreate for each test case.

So, what do I do?

Test the handlers in isolation?

slackermorris commented 6 months ago

But the handler depends on there being a jsonBodyParser otherwise I will have to write the test such that a stringified body is passed.

slackermorris commented 6 months ago

If I wanted to test the rawHandler I would have to:

  1. Modify the handler so that it isn't wrapped with middy middleware.
  2. Make a new named export in tested lambda such that it is now wrapped in a testHandler wrapper or equivalent.
slackermorris commented 6 months ago

Why does the body parser blow up? Haven't I seen this before?

slackermorris commented 6 months ago

I think that I did work around this using a rawHandler? But what about the error handling that I expect?

export const rawHandler = async (event: CreateUserEvent) => {
  const {
    body: { username },
  } = validateEvent(event);

  const user = await createUser(new User(username));

  return user;
};

export const handler = handlerWrapper<CreateUserEvent, User>(rawHandler, {
  httpJsonBodyParserEnabled: true,
});

Yeah, I completely lose the error handling from the initial wrapper. Not good. How else can I do this?

slackermorris commented 6 months ago

Which leaves me thinking that perhaps it is better to test with the middy middleware.

slackermorris commented 6 months ago

Well, now we have hit another hitch. The middy middleware that resolves the body as JSON obviously can't perform its duty under test because it depends on being fed a string. Sooo....

I really need to separate the handler from its wrappers. I need to test the raw body logic of the handler.

slackermorris commented 6 months ago

Either I write my tests such that they actually represent a likely Proxy Event, or I somehow extract and simplify the way error handling and middy middleware are put together.

slackermorris commented 6 months ago

I notice at work that we test the rawHandler and just verify that we throw errors etc instead of testing for the outcome that throwing that error results in, e.g. particular error message.

The approach I am taking currently really centres on testing the lambda as a whole.

slackermorris commented 6 months ago

This gives a little insight into how to unit test lambdas, but isn't as fleshed out as I need it to be: https://kalinchernev.github.io/tdd-serverless-jest

slackermorris commented 6 months ago

Perhaps I tested the error handling in isolation and it is better to do an integration test to verify that appropriate error handling is being managed.

slackermorris commented 6 months ago

Yeah, because testing http error codes does not seem verify appropriate. However, I then have the difficulty of having to explicitly manage errors. Meh.

I think most importantly I don't like the double handling of validation. Like, we can leverage zod as middy middleware to perform validation, so repeating this validation in the body of the handler is double handling.

To better understand this we can refer to src/endpoints/getVariationDefinitions/index.ts.

slackermorris commented 6 months ago

There are some serious testing lessons that I can peel from this.

slackermorris commented 6 months ago

Fuck what a supreme PITA.

I want to test that validation fails but because all the logic for better formatting the errors exists in the handler wrapper I can't do so. And it seems weird to pull this validation logic up any higher or closer to the handler itself.

slackermorris commented 6 months ago

Test the body logic of the lambda handler. Use integration tests to check for failed validation and proper error handling.

slackermorris commented 6 months ago

So, perhaps I write the validation check as part of middy middleware? I can take instruction from this package: https://github.com/lechodiman/middy-zod-validator

slackermorris commented 6 months ago

This type of test is more relevant to integration tests:

  test.only("should fail to create a user because it does not pass validation", async () => {
    // const response = await createResponse({
    //   ...mockBaseEvent,
    //   // @ts-ignore: Think "runtime". It is possible that this handler be invoked without any path parameters.
    //   body: {},
    // });

    //? anticipate the handler to just throw an error

    await expect(
      createResponse({
        ...mockBaseEvent,
        // @ts-ignore: Think "runtime". It is possible that this handler be invoked without any path parameters.
        body: {},
      })
    ).rejects.toThrow(
      "Uh oh, encountered a validation error. Error in 'body': 'username' should have been string, but got undefined."
    );

    // expect(response.statusCode).toBe(500);

    // expect(JSON.parse(response.body!).error).toBe(
    //   "Uh oh, encountered a validation error. Error in 'body': 'username' should have been string, but got undefined."
    // );
  });
slackermorris commented 6 months ago

I am trying to test that the handler for create-user has validation being run on it and confirm that Zod is actually performing its job as middleware.

slackermorris commented 6 months ago

Hmmm. The error handling is only for the lambda body logic not for sure Zod stuff, so I probably do need to throw an HTTP error.

slackermorris commented 5 months ago

None of the mocking for the DynamoClient is actually working.

slackermorris commented 5 months ago

I don't know why the mock isn't returning anything. It seems as though the resolve portion is not working properly.

slackermorris commented 5 months ago

For some reason, this works:

import {mockClient} from '../src';
import {DynamoDBClient, paginateQuery, QueryCommand} from '@aws-sdk/client-dynamodb';
import {
    DynamoDBDocumentClient, paginateQuery as documentPaginateQuery, QueryCommand as DocumentQueryCommand,
} from '@aws-sdk/lib-dynamodb';
import {marshall} from '@aws-sdk/util-dynamodb';

 test("mocks client", async () => {
    const mock = mockClient(DynamoDBDocumentClient);
    mock.on(QueryCommand).resolves({
      Items: [{ pk: "a", sk: "b" }],
    });

    const dynamodb = new DynamoDBClient({});
    const ddb = DynamoDBDocumentClient.from(dynamodb);
    console.log("🚀 ~ test ~ ddb:", ddb);

    const query = await ddb.send(
      new QueryCommand({
        TableName: "mock",
      })
    );

    expect(query.Items).toHaveLength(1);
    expect(query.Items?.[0]).toStrictEqual({ pk: "a", sk: "b" });
  });
slackermorris commented 5 months ago

I need to write this distinction up ASAP: https://stackoverflow.com/questions/57804745/difference-between-aws-sdk-dynamodb-client-and-documentclient

slackermorris commented 5 months ago

Well I don't think a mismatch between client and document client quite scratched the itch.

slackermorris commented 5 months ago

It's got me thinking about whether the Movie and Watchlist need to store information about the username if they have their PK SK defined.

What are the implications of removing this information?

slackermorris commented 5 months ago

Well, I think I found where I need to do some refactoring. There is an odd difference in what is required to create a Movie versus what is required when extracting it from the database. We only need username and watchlistId when creating the entry for the Movie.

slackermorris commented 5 months ago

I am having some difficulty around calling the http endpoint for add movie to watchlist. OK I figured out the issue. The middleware body parser was set after the validation middleware.

middyHandlerWrapper.use(zodValidate(eventSchema));

httpJsonBodyParserEnabled && middyHandlerWrapper.use(httpJsonBodyParser());

When in fact it should be:

httpJsonBodyParserEnabled && middyHandlerWrapper.use(httpJsonBodyParser());

middyHandlerWrapper.use(zodValidate(eventSchema));
slackermorris commented 5 months ago

I have two more endpoints list-watchlist-movies and update, which should be renamed to update-watchlist-movie.

slackermorris commented 5 months ago

I know I had this error before, relates to the ddbMock.

TypeError: AwsStub{ …(2) } is not a spy or a call to a spy!

I remember now. I am trying to use the wrong assertion utilities. See:

expect(ddbMock).toHaveBeenCalledWith(
      expect.objectContaining({
        TableName: "unified-test-table",
        IndexName: "GSI1",
        KeyConditionExpression: "GSI1PK = :gsi1pk",
        ExpressionAttributeValues: {
          ":gsi1pk": "watchlist#trial-user#8JWw9ZPsUtkD-14h0Fnzs",
        },
      })
    );

It's meant to be:

expect(ddbMock).toHaveReceivedCommandWith
slackermorris commented 5 months ago

I tried to invoke the get-watchlist-movies endpoint but because there is case sensitivity with what is stored in the DB and what is resolved using the entity classes.

slackermorris commented 5 months ago

Well, I tried to solve the above by changing the partition and sort keys on the table. No cigar. I think I will need to drop the database entirely and then redeploy. I am sure I have some notes somewhere on how to do this.

slackermorris commented 5 months ago

I really need to figure out how to properly handle updating Resources using SST.

slackermorris commented 5 months ago

The list-wachlist-movies is the most up-to-date endpoint.

slackermorris commented 5 months ago

Great. That endpoint works.