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

test: trcp recipes #1237

Merged
merged 12 commits into from
Jan 14, 2024
Merged

test: trcp recipes #1237

merged 12 commits into from
Jan 14, 2024

Conversation

UlianaPurtova
Copy link
Contributor

this is just adding tests to Trcp projects
I know this is not part of pipeline, but could be added later on

Screenshot 2024-01-07 at 11 51 55 AM

@julianpoy
Copy link
Owner

Nice! Really excited to have a test setup for the tRPC side!

I'll leave some comments shortly


it("creates a recipe", async () => {
const recipe = await trpc.recipes.createRecipe.mutate({
title: "Sliced cucumber",
Copy link
Owner

@julianpoy julianpoy Jan 7, 2024

Choose a reason for hiding this comment

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

[non-blocking] For all of these values, I'd love for us to use the library faker.js to randomize the values

});

expect(recipe.id).toMatch(
/^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89aAbB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't believe it's really necessary to test a pattern - as long as it returns a string, there's no reason to compare against a regex.

That said, if we still want to check against a regex anyway, we should move this regex to a util.

Comment on lines 38 to 42
await prisma.recipe.delete({
where: {
id: recipe.id,
},
});
Copy link
Owner

Choose a reason for hiding this comment

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

This relies on some internal knowledge of the implementation of the mutation, rather than just testing behavior.

Since UUIDs are generated randomly there's really no need to cleanup between tests. Imo this can be removed.

expect(recipe.id).toMatch(
/^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89aAbB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/,
);
expect(recipe.title).toEqual("Sliced cucumber");
Copy link
Owner

Choose a reason for hiding this comment

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

I'd love to see a check using prisma to test that a recipe was indeed created in the DB.

Comment on lines 65 to 77
export function randomEmail() {
return `${randomString(20)}@gmail.com`;
}

export function randomString(len) {
let chars = "abcdefghijklmnopqrstuvwxyz";

let str = [];
for (let i = 0; i < len; i++)
str.push(chars.charAt(Math.floor(Math.random() * (chars.length - 1))));

return str.join("");
}
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than these, lets use faker.js

@julianpoy
Copy link
Owner

The tests in the tRPC side of the project should be TypeScript (no JavaScript on the tRPC side of the project).

The tests should also sit next to the source code in question, rather than in a separate test/ directory

@UlianaPurtova
Copy link
Contributor Author

I see a CI failure, will investigate

@UlianaPurtova
Copy link
Contributor Author

UlianaPurtova commented Jan 10, 2024

turned off tropic tests as part of circleCI as part of this commit, there were no test there anyway, so no loss I'd think
will have to figure out how to run these as a hopefully next commit or maybe a bit later, Im not too familiar with CI :)

update: I've poked around in the setup and I don't think I'd be able to start these up as part of the CI

@julianpoy
Copy link
Owner

I've pushed a few commits to this PR. I've:

  • Added the tests to CI so CircleCI starts an instance of the API in the background and tests against that
  • Updated the URL the tests hit, such that similar to the backend tests they are intended to be run inside of the container
  • Broke the tests up into individual files, so that each set of tests goes with it's respective source code
  • Moved util/test.ts to testutils.ts
  • Updated a few of the test titles to reflect the test behavior

It can be merged in it's current state, but leaving open since this is your PR and I definitely want your sign-off prior to merge as it stands now.

I also believe there's a lot we can expand on here (though this is absolutely a great foundation). To name a few, there are no tests for error cases, and no tests for things like labels & filtering which are critical codepaths for these recipe queries & mutations.

@@ -53,6 +53,13 @@ jobs:
- run:
name: migrate
command: npx prisma migrate dev
- run:
name: start api
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥

@UlianaPurtova
Copy link
Contributor Author

thank you a lot for your feedback @julianpoy
much appreciated! please merge this work in and I'll start working on error cases next week!

@julianpoy julianpoy merged commit 03cf3bf into julianpoy:master Jan 14, 2024
4 checks passed
@UlianaPurtova UlianaPurtova deleted the artem branch January 22, 2024 00:06
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.

2 participants