Improve Test Coverage

[varshith257]

Purpose

This PR aims to increase the test coverage of the StarbaseDB project to 75%+ using Vitest. The improvements ensure meaningful tests that not only improve line coverage but also help detect potential issues by testing different scenarios and edge cases and fixed few of them too

Tasks

• Added unit tests

• Fixed found issues too

• [ ]

Verify

• Run the test suite using:

pnpm vitest run --coverage

• Confirm that all tests pass and coverage has increased

Before

After

/claim #71

[varshith257]

@Brayden PTAL. I will test each function cleanly, push the code from on and have fixed few issues found too

[varshith257]

@Brayden Test coverage is good so far and few issues have been found in the DELETE operation so I have commented. I will take a close look fix it and improve some more files' test coverage to reach ~85-90

[varshith257]

@Brayden It's weekend and I am out . I will push final remaining changes by tomorrow

[Brayden]

@varshith257 No worries. I was just updating your PR so it didn't have merge conflicts. I introduced a couple test changes so a report could be displayed on new PR's. I ran the test coverage here and figured I'd share:

Also do you recall what error you were getting for DELETE operations? Curious to what bug was uncovered!

[varshith257]

@Brayden I can't remember what exactly found as I kept in a notes in my desktop but what i remember is it throwing bad request instead executing delete operation

For example check this

        // it('should handle DELETE requests successfully', async () => { 
         //     vi.mocked(executeQuery).mockImplementation(async ({ sql }) => { 
         //         console.log('Mock executeQuery called with:', sql) 
  
         //         if (sql.includes('PRAGMA table_info(users)')) { 
         //             return [{ name: 'id', pk: 1 }] 
         //         } 
  
         //         return [] 
         //     }) 
         //     vi.mocked(executeTransaction).mockResolvedValue([]) 
  
         //     const request = new Request('http://localhost/rest/users/1', { 
         //         method: 'DELETE', 
         //     }) 
         //     const response = await liteRest.handleRequest(request) 
  
         //     console.log('DELETE Test Response:', response) 
  
         //     expect(response).toBeInstanceOf(Response) 
         //     expect(response.status).toBe(200) 
  
         //     const jsonResponse = (await response.json()) as { 
         //         result: { message: string } 
         //     } 
         //     console.log('Parsed JSON Response:', jsonResponse) 
  
         //     expect(jsonResponse.result).toEqual({ 
         //         message: 'Resource deleted successfully', 
         //     }) 
         // })

and this too

        // it('should return 400 for DELETE without ID', async () => { 
         //     const request = new Request('http://localhost/rest/users', { 
         //         method: 'DELETE', 
         //     }) 
         //     const response = await liteRest.handleRequest(request) 
  
         //     expect(response).toBeInstanceOf(Response) 
         //     expect(response.status).toBe(400) 
  
         //     const jsonResponse = (await response.json()) as { error: string } 
         //     expect(jsonResponse.error).toBe( 
         //         "Missing primary key value for 'id'" 
         //     ) 
         // }) 
 

It throwing 500 instead 400 above and delete query succes. I need to go another round of their implementation to validate whether I am wrongly testing the method or need a fix there. To capitalise i have commented and just console logged their results but here mock data receiving perfectly to the DELTE but when executed final result is getting undefined

[Brayden]

@varshith257 Wanted to check in and see if you had more ready to push up or not! :)

[varshith257]

@Brayden Sorry for the inconvenience. Actually I had on holiday from last 3 days so didn't push changes here. I am back and on the work

[varshith257]

@Brayden I will push the remainging changes by EOD. And maybe your eye on them would be good as some of tests are frequently failing as i am testing them wrong or maybe that tests we don't need

[Brayden]

@Brayden I will push the remainging changes by EOD. And maybe your eye on them would be good as some of tests are frequently failing as i am testing them wrong or maybe that tests we don't need

Of course @varshith257, happy to take a look at the tests and verify them 👍

[varshith257]

@Brayden This is the failing test I don't know where it going wrong here as id receiving as undefined after pasing request

[Brayden]

@Brayden This is the failing test I don't know where it going wrong here as id receiving as undefined after pasing request

I'll take a look at this!

[Brayden]

@varshith257 It looks like in the ./src/literest/index.test.ts file all the tests are for the external source but we should probably make it for the internal source. For the error you're seeing, the paths should require /rest/${schema}/${table}/${identifier} but right now it looks like you're just doing /rest/table/identifier and the operations don't have access to a schema value.

We should probably do:

1. Make the testing source internal instead of external
2. Add in main as the schema value in the URL path for each REST operation

For the delete operation since the schema is missing and the last path value is the identifier it's acting as if no primary key identifier was passed into it so it doesn't know exactly what to delete.

[varshith257]

I am writing tests for rls then we are good to go hit >75

[varshith257]

@Brayden Am I missing something here to test JOINS? Currently it returning the sql what we specified in test and not applying applyRLS to sql

[varshith257]

And here too for SELECT case the applyRLS isn't applying

[Brayden]

Is this loading the policies like you do in the loadPolicies test? It should check the policies and if a policy exists for a particular table (in this case users) then it should be applied. I don't see the policies in play in this test but I could be missing it.

[Brayden]

Oh wait I see it in the beforeEach above this 🤦

[Brayden]

Looks like if I convert the schema from "public" to an empty string I get this result for this test, which probably indicates an issue in the code that should handle default schemas.:

 FAIL  src/rls/index.test.ts > applyRLS - Query Modification > should modify SELECT queries with WHERE conditions
AssertionError: expected 'SELECT * FROM `users` WHERE (`users`.…' to contain 'WHERE `user_id` = \'user123\''

Expected: "WHERE `user_id` = 'user123'"
Received: "SELECT * FROM `users` WHERE (`users`.`user_id` = 'user123')"

 ❯ src/rls/index.test.ts:114:29
    112|     it('should modify UPDATE queries with additional WHERE clause', async () => {
    113|         const sql = "UPDATE users SET name = 'Bob' WHERE age = 25"
    114|         const modifiedSql = await applyRLS({
       |                             ^
    115|             sql,
    116|             isEnabled: true,

[varshith257]

@Brayden I think there are till related issues for rls please check if wriitten tests are missing something or the implemetation lacks it. I have also found related issues in import and do files too. Do I push the tests here or lets ignore for now. I am thinking to wrap up this bounty

[Brayden]

This serves as a great foundation for tests for the entire project. Moving forward with approving this to open up opportunities to start resolving some of the failed tests identified. Thanks for all of your hard work on this effort!