-
-
Notifications
You must be signed in to change notification settings - Fork 138
Add tests for less common entity routes #920
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { expect, test } from 'vitest' | ||
import { app } from './utils' | ||
|
||
test('config version endpoint', async () => { | ||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/config/version', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
const data = res.json() | ||
expect(data).toHaveProperty('version') | ||
expect(typeof data.version).toBe('string') | ||
// Accept any version string format | ||
expect(data.version).toContain('PostgreSQL') | ||
}) | ||
|
||
test('config with invalid endpoint', async () => { | ||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/config/invalid', | ||
}) | ||
expect(res.statusCode).toBe(404) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import { expect, test } from 'vitest' | ||
import { app } from './utils' | ||
|
||
test('extension list filtering', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we actually check that some extensions correctly returned? it's also not filtering, name a bit misleading |
||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/extensions?limit=5', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
const extensions = res.json() | ||
expect(Array.isArray(extensions)).toBe(true) | ||
expect(extensions.length).toBeLessThanOrEqual(5) | ||
}) | ||
|
||
test('extension with invalid id', async () => { | ||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/extensions/99999999', | ||
}) | ||
expect(res.statusCode).toBe(404) | ||
}) | ||
|
||
test('create extension with invalid name', async () => { | ||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/extensions', | ||
payload: { | ||
name: 'invalid_extension_name_that_doesnt_exist', | ||
schema: 'public', | ||
version: '1.0', | ||
cascade: false, | ||
}, | ||
}) | ||
expect(res.statusCode).toBe(400) | ||
}) | ||
|
||
test('delete extension with invalid id', async () => { | ||
const res = await app.inject({ | ||
method: 'DELETE', | ||
path: '/extensions/99999999', | ||
}) | ||
expect(res.statusCode).toBe(404) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import { expect, test } from 'vitest' | ||
import { app } from './utils' | ||
|
||
test('format SQL query', async () => { | ||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/query/format', | ||
payload: { query: "SELECT id,name FROM users WHERE status='ACTIVE'" }, | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
const formattedQuery = res.body | ||
expect(formattedQuery).toMatchInlineSnapshot(` | ||
"SELECT | ||
id, | ||
name | ||
FROM | ||
users | ||
WHERE | ||
status = 'ACTIVE' | ||
" | ||
`) | ||
}) | ||
|
||
test('format complex SQL query', async () => { | ||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/query/format', | ||
payload: { | ||
query: | ||
"SELECT u.id, u.name, p.title, p.created_at FROM users u JOIN posts p ON u.id = p.user_id WHERE u.status = 'ACTIVE' AND p.published = true ORDER BY p.created_at DESC LIMIT 10", | ||
}, | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
expect(res.body).toMatchInlineSnapshot(` | ||
"SELECT | ||
u.id, | ||
u.name, | ||
p.title, | ||
p.created_at | ||
FROM | ||
users u | ||
JOIN posts p ON u.id = p.user_id | ||
WHERE | ||
u.status = 'ACTIVE' | ||
AND p.published = true | ||
ORDER BY | ||
p.created_at DESC | ||
LIMIT | ||
10 | ||
" | ||
`) | ||
}) | ||
|
||
test('format invalid SQL query', async () => { | ||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/query/format', | ||
payload: { query: 'SELECT FROM WHERE;' }, | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
expect(res.body).toMatchInlineSnapshot(` | ||
"SELECT | ||
FROM | ||
WHERE; | ||
" | ||
`) | ||
}) | ||
|
||
// TODO(andrew): Those should return 400 error code for invalid parameter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe 200 and do nothing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agreed 200 is fine here. I think just like formatters in code editors it shouldn't fail when there are syntax errors; rather it should try to format the rest of the code despite the errors. |
||
test('format empty query', async () => { | ||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/query/format', | ||
payload: { query: '' }, | ||
}) | ||
expect(res.statusCode).toBe(500) | ||
}) | ||
|
||
test('format with missing query parameter', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: |
||
const res = await app.inject({ | ||
method: 'POST', | ||
path: '/query/format', | ||
payload: {}, | ||
}) | ||
expect(res.statusCode).toBe(500) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,81 @@ | ||||||
import { expect, test } from 'vitest' | ||||||
import { app } from './utils' | ||||||
|
||||||
test('function list filtering', async () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it should create functions first and check that they are correctly returned, if we test with limit, probably we should also create limit + 1 first and then check that limit actually works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah issue we currently have with this repo tests is that any changes to the schemas make side effect to all the other tests. We'll need to setup something like this first: https://github.com/supabase/supabase/blob/master/packages/pg-meta/test/db/utils.ts So each test can run in it's isolated database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that is not great, but it is not the only way, another one is “create functions - test - drop functions” There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue with that is that still cause conflicts depending on the orders the tests run in / parallel runs (sometimes the drop didn't happen while another introspection query will get a different result). I guess we can make the tests more synchronous to avoid that, even if I'm not sure how, maybe wrap them in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i mean, it prob wont conflict if you still check that there are exactly 5 functions and create 6 in advance, you dont have to check function names for a better test we can also query functions from the database directly and check that pg-meta returns a subset of the functions that we got using db query directly |
||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/functions?limit=5', | ||||||
}) | ||||||
expect(res.statusCode).toBe(200) | ||||||
const functions = res.json() | ||||||
expect(Array.isArray(functions)).toBe(true) | ||||||
expect(functions.length).toBeLessThanOrEqual(5) | ||||||
}) | ||||||
|
||||||
test('function list with specific included schema', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/functions?includedSchemas=public', | ||||||
}) | ||||||
expect(res.statusCode).toBe(200) | ||||||
const functions = res.json() | ||||||
expect(Array.isArray(functions)).toBe(true) | ||||||
// All functions should be in the public schema | ||||||
functions.forEach((func) => { | ||||||
expect(func.schema).toBe('public') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should create some first in other schemas |
||||||
}) | ||||||
}) | ||||||
|
||||||
test('function list exclude system schemas', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/functions?includeSystemSchemas=false', | ||||||
}) | ||||||
expect(res.statusCode).toBe(200) | ||||||
const functions = res.json() | ||||||
expect(Array.isArray(functions)).toBe(true) | ||||||
// No functions should be in pg_ schemas | ||||||
functions.forEach((func) => { | ||||||
expect(func.schema).not.toMatch(/^pg_/) | ||||||
}) | ||||||
}) | ||||||
|
||||||
test('function with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/functions/99999999', | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) | ||||||
|
||||||
test('create function with invalid arguments', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'POST', | ||||||
path: '/functions', | ||||||
payload: { | ||||||
name: 'invalid_function', | ||||||
schema: 'public', | ||||||
// Missing required args | ||||||
}, | ||||||
}) | ||||||
expect(res.statusCode).toBe(400) | ||||||
}) | ||||||
|
||||||
test('update function with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'PATCH', | ||||||
path: '/functions/99999999', | ||||||
payload: { | ||||||
name: 'renamed_function', | ||||||
}, | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) | ||||||
|
||||||
test('delete function with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'DELETE', | ||||||
path: '/functions/99999999', | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { expect, test } from 'vitest' | ||
import { app } from './utils' | ||
|
||
test('typescript generator route', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not actually testing anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would actually disagree, even if minimal this ensure that the route doesn't throw an exception (which occurs if invalid TS is generated when passing within prettier for instance, or if there is a bug in the JS within the string interpolations used to generate the content). That's not a great test, but that's a start. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and if due to code changes it this route starts to return no types instead of an error, can you spot it with this test? this test can be sort of easily improved if you have a file with types saved somewhere and you compare the body with the template file (it can even be contains and some expected parts) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's not great. I guess in the case you mention "public" will probably not be present in the "schemas" hence fail the test. But I agree that's brittle. Looking at your suggestion this might be a good place to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can just create a table, a function, a view, etc in the beginning of the test and check that types are generated as expected for these |
||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/generators/typescript', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
expect(res.body).contain('public') | ||
}) | ||
|
||
test('go generator route', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same, not a test |
||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/generators/go', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
expect(res.body).toBeTruthy() | ||
}) | ||
|
||
test('swift generator route', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same, misses essential checks |
||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/generators/swift', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
expect(res.body).toBeTruthy() | ||
}) | ||
|
||
test('generator routes with includedSchemas parameter', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. valid, but a lot of actual issues in the code may be missed, would be better to include one or a few of multiple existing schemas and check that only they are included |
||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/generators/typescript?included_schemas=private', | ||
}) | ||
expect(res.statusCode).toBe(200) | ||
expect(res.headers['content-type']).toContain('text/plain') | ||
// the only schema is excluded database should be empty | ||
expect(res.body).toContain('Database = {}') | ||
}) | ||
|
||
test('invalid generator route', async () => { | ||
const res = await app.inject({ | ||
method: 'GET', | ||
path: '/generators/openapi', | ||
}) | ||
expect(res.statusCode).toBe(404) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,71 @@ | ||||||
import { expect, test } from 'vitest' | ||||||
import { app } from './utils' | ||||||
|
||||||
test('policy list filtering', async () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also same as for other list tests, we should create some first to check that they are returned and that limit works |
||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/policies?limit=5', | ||||||
}) | ||||||
expect(res.statusCode).toBe(200) | ||||||
const policies = res.json() | ||||||
expect(Array.isArray(policies)).toBe(true) | ||||||
expect(policies.length).toBeLessThanOrEqual(5) | ||||||
}) | ||||||
|
||||||
test('policy list with specific included schema', async () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to create some in public and in other schema(s) first (we prob have only 1) |
||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/policies?included_schema=public', | ||||||
}) | ||||||
expect(res.statusCode).toBe(200) | ||||||
const policies = res.json() | ||||||
expect(Array.isArray(policies)).toBe(true) | ||||||
// All policies should be in the public schema | ||||||
policies.forEach((policy) => { | ||||||
expect(policy.schema).toBe('public') | ||||||
}) | ||||||
}) | ||||||
|
||||||
test('policy with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'GET', | ||||||
path: '/policies/99999999', | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) | ||||||
|
||||||
test('create policy with missing required field', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'POST', | ||||||
path: '/policies', | ||||||
payload: { | ||||||
name: 'test_policy', | ||||||
schema: 'public', | ||||||
// Missing required table field | ||||||
definition: 'true', | ||||||
check: 'true', | ||||||
action: 'SELECT', | ||||||
command: 'PERMISSIVE', | ||||||
}, | ||||||
}) | ||||||
expect(res.statusCode).toBe(400) | ||||||
}) | ||||||
|
||||||
test('update policy with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'PATCH', | ||||||
path: '/policies/99999999', | ||||||
payload: { | ||||||
name: 'renamed_policy', | ||||||
}, | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) | ||||||
|
||||||
test('delete policy with invalid id', async () => { | ||||||
const res = await app.inject({ | ||||||
method: 'DELETE', | ||||||
path: '/policies/99999999', | ||||||
}) | ||||||
expect(res.statusCode).toBe(404) | ||||||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chore
Avoid
ident
to raise an exception ending with500
if those params are empty. TODO: migrate all this to zod for better validation of the params.