Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork147
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedApr 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Pull Request Test Coverage Report forBuild 14224011078Details
💛 -Coveralls |
| if (!table || !name) { | ||
| return { data: null, error: { message: 'Missing required name or table parameter' } } | ||
| } |
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
Avoidident to raise an exception ending with500 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.
Choose a reason for hiding this comment
The 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
| `) | ||
| }) | ||
| // TODO(andrew): Those should return 400 error code for invalid parameter |
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.
maybe 200 and do nothing?
empty string isnt smth bad really
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| test('function listfiltering',async()=>{ | |
| test('function listwith limit',async()=>{ |
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.
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 comment
The 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 comment
The 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 comment
The 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 indescribe ?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should create some first in other schemas
| }) | ||
| }) | ||
| test('trigger with invalid id', async () => { |
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.
| test('trigger with invalid id',async()=>{ | |
| test('gettrigger with invalid id',async()=>{ |
| expect(types.length).toBeLessThanOrEqual(5) | ||
| }) | ||
| test('type list with specific included schema', async () => { |
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.
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.
Choose a reason for hiding this comment
The 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 (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| test('type listfiltering',async()=>{ | |
| test('type listwith 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do we have array types?
sorry if missed it
Uh oh!
There was an error while loading.Please reload this page.
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%