Conversation
Pull Request Test Coverage Report for Build 14224011078Details
💛 - Coveralls |
| if (!table || !name) { | ||
| return { data: null, error: { message: 'Missing required name or table parameter' } } | ||
| } |
There was a problem hiding this comment.
chore
Avoid ident to raise an exception ending with 500 if those params are empty. TODO: migrate all this to zod for better validation of the params.
| import { expect, test } from 'vitest' | ||
| import { app } from './utils' | ||
|
|
||
| test('extension list filtering', async () => { |
There was a problem hiding this comment.
should we actually check that some extensions correctly returned?
it's also not filtering, name a bit misleading
| `) | ||
| }) | ||
|
|
||
| // TODO(andrew): Those should return 400 error code for invalid parameter |
There was a problem hiding this comment.
maybe 200 and do nothing?
empty string isnt smth bad really
There was a problem hiding this comment.
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.
| expect(res.statusCode).toBe(500) | ||
| }) | ||
|
|
||
| test('format with missing query parameter', async () => { |
There was a problem hiding this comment.
todo:
this one should be 400 probably
| import { expect, test } from 'vitest' | ||
| import { app } from './utils' | ||
|
|
||
| test('function list filtering', async () => { |
There was a problem hiding this comment.
| test('function list filtering', async () => { | |
| test('function list with limit', async () => { |
There was a problem hiding this comment.
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.
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.
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.
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 describe ?
There was a problem hiding this comment.
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
| 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.
we should create some first in other schemas
| }) | ||
| }) | ||
|
|
||
| test('trigger with invalid id', async () => { |
There was a problem hiding this comment.
| test('trigger with invalid id', async () => { | |
| test('get trigger with invalid id', async () => { |
| expect(types.length).toBeLessThanOrEqual(5) | ||
| }) | ||
|
|
||
| test('type list with specific included schema', async () => { |
There was a problem hiding this comment.
we need to create some in other schema(s) first
| import { expect, test } from 'vitest' | ||
| import { app } from './utils' | ||
|
|
||
| test('type list filtering', async () => { |
There was a problem hiding this comment.
also same as for other list tests, we should create some first to check that they are returned and that limit works (not sure we have >5)
| import { expect, test } from 'vitest' | ||
| import { app } from './utils' | ||
|
|
||
| test('type list filtering', async () => { |
There was a problem hiding this comment.
| test('type list filtering', async () => { | |
| test('type list with limit', async () => { |
| const types = res.json() | ||
| expect(Array.isArray(types)).toBe(true) | ||
| // Should not include array types | ||
| const arrayTypes = types.filter((type) => type.name.startsWith('_')) |
There was a problem hiding this comment.
do we have array types?
sorry if missed it
5 participants
What kind of change does this PR introduce?
WARNING! LLM Generated, please review carefully
Test coverage for
server/config
server/extensions
server/format
server/functions
server/generators
server/policies
server/publications
server/triggers
server/types
to get coverage > 80%