#8 Creating database and user & granting user privileges to database #2
Conversation
allopez7
changed the title
database.js
Outdated
| }); | ||
|
|
||
| client.connect().then(() => { | ||
| createPgAccount("username", "password", "database"); |
There was a problem hiding this comment.
calling the function createPgAccount and passing in dummy data as arguments for now. Eventually, data from the sign-up page will get passed in.
There was a problem hiding this comment.
remove, or else this script will throw error.
database.js
Outdated
|
|
||
| const userDatabasePrivileges = await client.query(`GRANT ALL PRIVILEGES ON DATABASE ${database} To ${username}`) | ||
|
|
||
| await client.end() |
There was a problem hiding this comment.
why is your client ending when you've created 1 account? By doing this, you can't run this function more than once.
database.js
Outdated
|
|
||
| const createPgAccount = async (username, password, database) => { | ||
| try{ | ||
| const createDatabase = await client.query(`CREATE DATABASE ${database}`) |
There was a problem hiding this comment.
Why do you need to assign the return the value to variables when it's not being used?
It should just be await client.query(CREATE DATABASE ${database})
database.js
Outdated
|
|
||
| client.connect().then(() => { | ||
| console.log('connected') | ||
| }); |
There was a problem hiding this comment.
Put these inside a startPgDB function.
There was a problem hiding this comment.
Add comment
// when the app starts, we should first start all the databases we support (ie PostGres, MongoDb, Neo4J). If the DB can't start up, then our service is useless, so don't even start the app.
package.json
Outdated
| }, | ||
| "scripts": { | ||
| "test": "jest", | ||
| "start": "node database.js" |
There was a problem hiding this comment.
node index.js <- usually the default
database/postgres/pg.js
Outdated
| try{ | ||
| await client.end() | ||
| }catch(err){ | ||
| console.log('connection failed to closed', err) |
There was a problem hiding this comment.
this should return an error so whomever calls closePGDB will know that it has failed.
There was a problem hiding this comment.
please add this as a test case
database/postgres/pg.test.js
Outdated
| describe('Test startPGDB && closePGDB', ()=>{ | ||
| it('startPGDB', async ()=>{ | ||
| const startRes = await startPGDB() | ||
| if(startRes && startRes.error && startRes.error.message){ |
database/postgres/pg.test.js
Outdated
| expect(mockClient.query).toHaveBeenNthCalledWith(3, `GRANT ALL PRIVILEGES ON DATABASE username TO username`) | ||
| expect(async ()=>{ | ||
| await createPgAccount() | ||
| }).not.toThrow() |
There was a problem hiding this comment.
expecting it to not throw is not doing anything. because your test doesn't have try catch. If the function threw something, it would not pass
database/postgres/pg.test.js
Outdated
| expect(mockClient.query).toHaveBeenNthCalledWith(2, `DROP USER IF EXISTS username`) | ||
| expect(async ()=>{ | ||
| await deletePgAccount('username') | ||
| }).not.toThrow() |
There was a problem hiding this comment.
expecting it to not throw is not doing anything. because your test doesn't have try catch. If the function threw something, it would not pass
database/postgres/pg.test.js
Outdated
| }) | ||
| }) | ||
| describe('Test createPgAccount', ()=>{ | ||
| it('createPgAccount', async ()=>{ |
There was a problem hiding this comment.
should run all necessary queries in a successful createPgAccount call
database/postgres/pg.test.js
Outdated
| expect(mockClient.query).toHaveBeenNthCalledWith(3, `GRANT ALL PRIVILEGES ON DATABASE username TO username`) | ||
| await closePGDB() | ||
| }) | ||
| it('createPgAccount, undefined parameters', async ()=>{ |
There was a problem hiding this comment.
it should not execute any queries if required arguments are not passed in.
database/postgres/pg.test.js
Outdated
| expect(mockClient.query).toHaveBeenNthCalledWith(2, `DROP USER IF EXISTS username`) | ||
| await closePGDB() | ||
| }) | ||
| it('deletePgAccount, undefined parameters', async ()=>{ |
There was a problem hiding this comment.
it should not execute any queries in deletePgAccount if required arguments are not passed in.
database/postgres/pg.test.js
Outdated
| }) | ||
| }) | ||
| describe('Test deletePgAccount', ()=>{ | ||
| it('deletePgAccount', async ()=>{ |
There was a problem hiding this comment.
it should execute all queries if required arguments are passed into deletePgAccount
database/postgres/pg.test.js
Outdated
| } | ||
| let {startPGDB, closePGDB, createPgAccount, deletePgAccount} = require('./pg') | ||
| describe('Test startPGDB && closePGDB', ()=>{ | ||
| it('startPGDB', async ()=>{ |
There was a problem hiding this comment.
should call connect when starting PGDB
database/postgres/pg.test.js
Outdated
| await startPGDB() | ||
| expect(mockClient.connect).toHaveBeenCalledTimes(1) | ||
| }) | ||
| it('closePGDB', async ()=>{ |
There was a problem hiding this comment.
should call end when starting PGDB
database/postgres/pg.test.js
Outdated
| @@ -0,0 +1,61 @@ | |||
| const pgClient = require('pg') | |||
There was a problem hiding this comment.
jest.mock('pg')
const {Client} = require('pg')
const {startPGDB.....} = require('./pg')
database/postgres/pg.test.js
Outdated
| this.query = mockClient.query | ||
| this.connect = mockClient.connect | ||
| this.end = mockClient.end | ||
| } |
There was a problem hiding this comment.
in addition to comment (above), you can do this to be more concise:
Client.mockImplementation(function() {
return mockClient
})
database/postgres/pg.test.js
Outdated
| const {startPGDB, closePGDB, createPgAccount, deletePgAccount} = require('./pg') | ||
|
|
||
| describe('Test PG DB', ()=>{ | ||
| afterEach(()=>{ |
There was a problem hiding this comment.
This should be beforeEach(...) shoudn't it?
There was a problem hiding this comment.
It could be beforeEach as well but on the first test the mocks would be cleared when there is no need to clear.
There was a problem hiding this comment.
should be beforeEach. As the test suite gets more complex, this will cause issues later on, you can't rely on other ppl to always clear their test.
database/postgres/pg.test.js
Outdated
| Client.mockImplementation(function(){ | ||
| return mockClient | ||
| }) | ||
| global.console.log = jest.fn() |
There was a problem hiding this comment.
I think it's console.log = jest.fn(), also it'll be clearer if it moves to under 47th line
There was a problem hiding this comment.
but that wouldn't make sense for line 75 where it's called again. Since it's global i'm changing the functionality of console so I wanted to make it clear that this is changing the functionality of console for the whole database file. You do you have a good point so I'm going to take it outside of the postgres test so we can use it in the MongoDB and Neo4J test as well.
There was a problem hiding this comment.
this should be scoped into the function itself. Its not good for a developer later on to try to console.log something, but nothing shows up. This will essentially remove console.log functionality, which will remove debugging feature for alot of devs. Better to be inside the test.
allopez7
changed the title
No description provided.