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

dev: api integration tests #907

Merged
merged 7 commits into from
Jan 22, 2025
Merged

dev: api integration tests #907

merged 7 commits into from
Jan 22, 2025

Conversation

tefkah
Copy link
Contributor

@tefkah tefkah commented Jan 21, 2025

  • feat: api tests
  • fix: types and tests
  • fix: add common 400 response type

Issue(s) Resolved

Mostly want this before implementing #887 in the API.

High-level Explanation of PR

Adds integration tests for the API. For now just very basic tests: create a pub, get a pub. Should add more in depth tests later.

Roughly:

  1. Creates an API token through the UI
  2. Initializes a ts-rest client
  3. Makes some requests.

Other than the initial setup, these tests are quite fast as they don't really use the UI at all.

I chose not to use playwrights built in API client, as it's not typed and is more error prone when it comes to routes. The goal is more to test API responses rather than how it interacts with any client.

Test Plan

Screenshots (if applicable)

Notes

@tefkah tefkah changed the title feat: api intergration tests feat: api integration tests Jan 21, 2025
Comment on lines +64 to +67
defaultValues: {
// default to 1 day from now, mostly to make testing easier
expiration: new Date(Date.now() + 1000 * 60 * 60 * 24),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't want to write the code to navigate the date picker, so i took the easy way out and set a default time instead. let me know if the default time should be longer or if we should do this differently.

@@ -530,12 +548,19 @@ export const siteApi = contract.router(
},
},
{
strictStatusCodes: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes the return types from the api client more legible. instead of getting

type Response = 
| { status: 200, body: {...}} 
| { status: 201 | 202 | ...., body: any} 

you get

type Response = { status: 200, body: {...}} 

Comment on lines +559 to +564
commonResponses: {
// this makes sure that 400 is always a valid response code
400: zodErrorSchema,
403: z.string(),
404: z.string(),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

did not know this was an option, now every route has the correct error responses at least.

});

client = initClient(siteApi, {
baseUrl: `http://localhost:3000/`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might need to be an env var

@tefkah tefkah marked this pull request as ready for review January 21, 2025 12:42
@tefkah tefkah requested review from kalilsn, 3mcd and allisonking and removed request for kalilsn January 21, 2025 12:42
@tefkah tefkah changed the title feat: api integration tests dev: api integration tests Jan 21, 2025
Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

woot, awesome!!

@tefkah tefkah merged commit 404bf17 into main Jan 22, 2025
6 checks passed
@tefkah tefkah deleted the tfk/api-tests branch January 22, 2025 10:25
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.

3 participants