adaltas / ece-devops-2022-fall

Resources for the ECE 2022 DevOps course
66 stars 72 forks source link

(Lab 3) Can't perform 'get a user by username' test due to db flushed #1

Closed maximepires4 closed 1 year ago

maximepires4 commented 1 year ago

In test/user.controller.js, the beforeEach hook which flushes the database seems to make it impossible to create a user using a post method and then trying to get it using a get method.

Example :

    describe('GET /user', ()=> {
       it('get a user by username', (done) => {
           const user = {
               username: 'testuser',
               firstname: 'test',
               lastname: 'user'
           }

            chai.request(app)
               .post('/user')
               .send(user)
               .then(() => {

                   // At this point, the db seems to be flushed somehow

                   chai.request(app)
                       .get('/user/' + user.username)
                       .then((res) => {
                           // Here the res is null
                           // ...
                           done()
                       })
                   }
               )
       })
   })
})

The code above works if I comment the line db.flushdb() in the beforeEach hook. I would guess the line chai.request(app) calls this hook.

Is there a workaround or something I didn't understand?

I tested using npm start and executing POST and GET queries trough Insomnia. It worked just fine, the GET returned the content of the user sent in POST.

sergkudinov commented 1 year ago

chai.request(app) doesn't call this hook, it is called just before it() starts. In your case, you probably don't terminate the post() method correctly in your route, or there is another issue in your callback function.

This works for me:

    it.only('get an existing user', (done) => {
      const user = {
        username: 'sergkudinov',
        firstname: 'Sergei',
        lastname: 'Kudinov'
      }
      // Create a user
      chai.request(app)
        .post('/user')
        .send(user)
        .then(() => {
          chai.request(app)
            .get('/user/' + user.username)
            .then((res) => {
              chai.expect(res.body.msg.firstname).to.equal(user.firstname)
              done()
            })
        })
    })

Another way is to use a controller method to prepare you desired state of DB rather than sending HTTP requests:

   it.only('get an existing user', (done) => {
      const user = {
        username: 'sergkudinov',
        firstname: 'Sergei',
        lastname: 'Kudinov'
      }
      userController.create(user, () => {
        // Get the user
        chai.request(app)
          .get('/user/' + user.username)
          .then((res) => {
            chai.expect(res).to.have.status(200)
            chai.expect(res.body.status).to.equal('success')
            chai.expect(res).to.be.json
            done()
          })
          .catch((err) => {
             throw err
          })
      })
    })

The reason is gaining speed of tests. Refer to test writing best practicies.

wdavidw commented 1 year ago

Be carefull when using Express.js, if I recall corectly, thrown errors in the routes are not catched.

Also, use async/await instead of .then().catch(). This way, if you forget to catch an error, it will still be thrown. Code is more robust and readable:

    it.only('get an existing user', async () => {
      const user = {
        username: 'sergkudinov',
        firstname: 'Sergei',
        lastname: 'Kudinov'
      }
      // Create a user
      await chai.request(app)
        .post('/user')
        .send(user)
      const {body} = await chai.request(app)
        .get('/user/' + user.username)
      chai.expect(body.msg.firstname).to.equal(user.firstname)
    })
maximepires4 commented 1 year ago

Thanks for you answer @sergkudinov .

In your case, you probably don't terminate the post() method correctly in your route, or there is another issue in your callback function.

This advice did ringed a bell, in fact I was using a lambda function with 3 parameters on my route's get() method (the third argument is next). Ad the end of the function, I was calling next(). I just deleted the next argument and the final call, it is now working fine.

Only, I wonder why deleting the flushdb in the hook resolved my problem earlier.

I have a little question though about your second code. As the functions inside the variable userController are tested in the user.controller.js file, is it not unsafe to make a call to something we don't know if it has been tested yet?

maximepires4 commented 1 year ago

Thanks for the advice @wdavidw

wdavidw commented 1 year ago

is it not unsafe to make a call to something we don't know if it has been tested yet

I understand as, should I test user.controller in my current test. If so, that a fundamental concern:

A unit test, is and should only be concerned about its nature, what it is supposed to test. If you start asserting other concerns, your code begins to be messy and you stop trusting it. If you do it properly, you gain confidence in your tests, thus in your code. You can work on it for years even after months/years of interruption.

maximepires4 commented 1 year ago

Thank you for the answer @wdavidw I am closing the issue since you answered all my questions.