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

add tests for HTTP APIs and their routes #30

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

akhileshthite
Copy link
Collaborator

@akhileshthite akhileshthite commented Dec 18, 2023

✘ [fail]: server › api › admins › GET /admins - success
✔ server › api › admins › GET /admins - unauthorized (2.9s)
✘ [fail]: server › api › admins › POST /admins - success
✔ server › api › admins › POST /admins - unauthorized (1.2s)
✘ [fail]: server › api › admins › DELETE /admins - success
✔ server › api › admins › DELETE /admins - unauthorized

  server › api › admins › GET /admins - success

  src/server/api/admins.test.ts:79

   78:                                                                
   79:   t.is(response.statusCode, 200)                               
   80:   t.is(response.body, '[email protected]\[email protected]')

  Difference (- actual, + expected):

  - 403
  + 200

  › file://src/server/api/admins.test.ts:79:5



  server › api › admins › POST /admins - success

  src/server/api/admins.test.ts:107

   106:                                 
   107:   t.is(response.statusCode, 200)
   108: })                              

  Difference (- actual, + expected):

  - 403
  + 200

  › file://src/server/api/admins.test.ts:107:5



  server › api › admins › DELETE /admins - success

  src/server/api/admins.test.ts:138

   137:                                 
   138:   t.is(response.statusCode, 200)
   139: })                              

  Difference (- actual, + expected):

  - 403
  + 200

  › file://src/server/api/admins.test.ts:138:5

  ─

  3 tests failed

@akhileshthite akhileshthite self-assigned this Dec 18, 2023
src/server/api/admins.test.ts Outdated Show resolved Hide resolved
src/server/api/admins.test.ts Outdated Show resolved Hide resolved
src/server/api/admins.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

Looking great! It'd be good to add a bit more verification of effects beyond checking that the HTTP status code is 200 so we can catch false positives.

src/server/api/admins.test.ts Show resolved Hide resolved
// t.is(response.body, '[email protected]', 'returns the blocklist')
// })

test.serial('POST /blocklist - success', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check that either the add method was called or fetch the list to make sure it is populated. Same for the other tests

Copy link
Collaborator Author

@akhileshthite akhileshthite Jan 22, 2024

Choose a reason for hiding this comment

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

Resolved for GET /blocklist and GET /allowlist, but getting errors for GET /:actor/blocklist and GET /:actor/allowlist, can you check?



  ✘ [fail]: server › api › blockallowlist › GET /:actor/blocklist - success returns the correct blocklist
  ✘ [fail]: server › api › blockallowlist › GET /:actor/allowlist - success returns the correct allowlist

  server › api › blockallowlist › GET /:actor/blocklist - success

  src/server/api/blockallowlist.test.ts:253

   252:   t.is(response.statusCode, 200, 'returns a status code of 200')                          
   253:   t.deepEqual(response.body.split('\n'), blockedAccounts, 'returns the correct blocklist')
   254: })                                                                                        

  returns the correct blocklist

  Difference (- actual, + expected):

    [
  -   '',
  +   '[email protected]',
  +   '[email protected]',
    ]

  › file://src/server/api/blockallowlist.test.ts:253:5



  server › api › blockallowlist › GET /:actor/allowlist - success

  src/server/api/blockallowlist.test.ts:300

   299:   t.is(response.statusCode, 200, 'returns a status code of 200')                          
   300:   t.deepEqual(response.body.split('\n'), allowedAccounts, 'returns the correct allowlist')
   301: })                                                                                        

  returns the correct allowlist

  Difference (- actual, + expected):

    [
  -   '',
  +   '[email protected]',
  +   '[email protected]',
    ]

  › file://src/server/api/blockallowlist.test.ts:300:5

  ─

  2 tests failed

Copy link
Contributor

Choose a reason for hiding this comment

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

Akhilesh Thite
: I think the issue with the foractor blocklist in pr #30 is that forActor returns a new object each time so you can't really stub it

@@ -92,6 +92,6 @@ export const creationRoutes = (cfg: APIConfig, store: Store, apsystem: ActivityP
}

await store.forActor(actor).delete()
return await reply.send({ message: 'Data deleted successfully' })
return await reply.code(200).type('application/json').send(JSON.stringify({ message: 'Data deleted successfully' }))
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the behavior that needed this change? .send(object) should already set the status to 200 and the content type to json. https://fastify.dev/docs/latest/Reference/Reply/#objects

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

return await reply.send({ message: 'Data deleted successfully' }) was giving this error:

✘ [fail]: server › api › creation › DELETE /:actor - success returns success message

  server › api › creation › DELETE /:actor - success

  src/server/api/creation.test.ts:124

   123:   // Examine the response body directly                                                                   
   124:   t.deepEqual(deleteResponse.json(), { message: 'Data deleted successfully' }, 'returns success message');
   125: });                                                                                                       

  returns success message

  Difference (- actual, + expected):

  - '[object Object]'
  + {
  +   message: 'Data deleted successfully',
  + }

  › file://src/server/api/creation.test.ts:124:5

  ─

  1 test failed

t.context.server = await spawnTestServer()

// Set up the mockStore
t.context.mockStore = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting up a mock store, try using a regular Store with in-memory storage, then instead of stubbing you could add/remove/list expected data.

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.

2 participants