freeCodeCamp / pantry-for-good

An open source food bank logistics and inventory management tool
Other
395 stars 189 forks source link

Tests #130

Open jspaine opened 7 years ago

jspaine commented 7 years ago

Both the server and client portions of the application need more test coverage

dmcfaddengalway commented 7 years ago

Hey @kenjiO , I'd love to help out with this. I've been learning about unit tests recently and want to see what I can apply. Let me know if this is free to try.

jspaine commented 7 years ago

Hi, there's quite a lot left to test so just do some if you want. The chances of someone else submitting tests for the same thing at the same time are pretty slim I think. Just make a pull request for every few you do so it stays fairly up to date.

Thirunavukkarasu commented 6 years ago

@jspaine I'm interested in writing test cases for some the remaining modules. Please let me know for which i'm allowed try.

Thanks!

kenjiO commented 6 years ago

You are welcome to test anything you find that you think needs more testing. You can run the npm run coverage task an it will generate a test report in coverage/lcov-report/index.html that shows which parts of the codes the tests are touching. If you find something you want to test I would just open up an issue for the specific files you are testing so no one else works on it.

Thirunavukkarasu commented 6 years ago

@kenjiO - I will be writing tests for questionnaire-view and section-view.

QuestionnaireView.js SectionView.js

Thanks.

vanessasena commented 6 years ago

Hi, when I execute npm run test I got the error "Fatal RangeError: Maximum call stack size exceeded". Does anyone know what is this?

jspaine commented 6 years ago

Hi, can you post the stacktrace, and whether it happens in client or server tests? (npm run test:client and npm run test:server)

vanessasena commented 6 years ago

Hi @jspaine the problem is in the server, in this test. It is getting into an infinite loop, but I don't know why.

Fatal RangeError: Maximum call stack size exceeded at trim_prefix (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:288:23) at /home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:335:12) at next (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:275:10) at Layer.handle_error (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/layer.js:67:12) at trim_prefix (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:315:13) at /home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:335:12) at next (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:275:10) at Layer.handle_error (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/layer.js:67:12) at trim_prefix (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:315:13) at /home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:284:7 . . . . at Function.process_params (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:335:12) at next (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:275:10) at /home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:635:15 at next (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/index.js:260:14) at next (/home/vanessa/codes/Pantry-for-Good/node_modules/express/lib/router/route.js:127:14) at /home/vanessa/codes/Pantry-for-Good/node_modules/express-promise-router/lib/express-promise-router.js:47:17 at tryCatcher (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/util.js:16:23) at Promise._settlePromiseFromHandler (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/promise.js:512:31) at Promise._settlePromise (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/promise.js:569:18) at Promise._settlePromise0 (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/promise.js:614:10) at Promise._settlePromises (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/promise.js:689:18) at Async._drainQueue (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/async.js:133:16) at Async._drainQueues (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/async.js:143:10) at Immediate.Async.drainQueues (/home/vanessa/codes/Pantry-for-Good/node_modules/bluebird/js/release/async.js:17:14) at runCallback (timers.js:789:20) at tryOnImmediate (timers.js:751:5) at processImmediate [as _immediateCallback] (timers.js:722:5) npm ERR! code ELIFECYCLE npm ERR! errno 2 npm ERR! pantry-for-good@0.0.1 test:server:once: cross-env NODE_ENV=test mocha --timeout 5000 --require babel-register --require server/entry.test.js "server/**/*.spec.js" "--watch" npm ERR! Exit status 2 npm ERR! npm ERR! Failed at the pantry-for-good@0.0.1 test:server:once script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in: npm ERR! /home/vanessa/.npm/_logs/2017-12-21T17_27_01_484Z-debug.log npm ERR! code ELIFECYCLE npm ERR! errno 2 npm ERR! pantry-for-good@0.0.1 test:server: npm run test:server:once -- --watch npm ERR! Exit status 2 npm ERR! npm ERR! Failed at the pantry-for-good@0.0.1 test:server script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in: npm ERR! /home/vanessa/.npm/_logs/2017-12-21T17_27_01_875Z-debug.log

jspaine commented 6 years ago

That's weird, do any other tests fail?

What if you call http://localhost:3000/api/questionnaires/5a3bfa755c7f858e8211b6f3 from a browser, does it also crash the server?

vanessasena commented 6 years ago

@jspaine all the other tests are passing

In the browser it is not crashing. It is returning status 404 and the error message Not found.

akbefu commented 6 years ago

When I run npm run test:server the Customer Api test fails for me with a timeout error that says this: 1) Customer Api "before all" hook: Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

If I override the timeout for the Customer Api tests by adding this.timeout(10000) to the beginning of the before method, then the test passes. Does anyone else experience this?

I also experience timeouts when running the npm run coverage command. This looks to be because of a decreased timeout (down to 2,000ms from 5,000ms when run with the npm run test:server command).

redbow-kimee commented 6 years ago

Hello! I'm looking into writing some unit tests, and before I start I wanted to ask if there's anything with priority for testing or that is already being worked?

kenjiO commented 6 years ago

If you want to write some unit tests you can work on tests for the components in these directories

client/modules/core/components client/modules/core/components/navbar client/modules/core/components/sidebar client/modules/customer/components client/modules/settings/components

I would look through some of the other tests and write yours in a similar way to keep things consistent. Thanks!

redbow-kimee commented 6 years ago

sure thing. I can get started with those.

redbow-kimee commented 6 years ago

Hello, I've made a start on writing some tests. I started with the Title element and the Footer element in client/modules/core/components (because they looked easy, and I'm relatively new to ReactJS). But I've come to the point where I'm running my head into a wall trying to mock some of the dependencies.

I'm attaching some stack traces, with reference to these files: https://github.com/RedBow/pantry-for-good/blob/testing/client-core-components/client/modules/core/components/Title.spec.js The main issue being that I can't get the Title to pick up my Sinon mocks.

and with this one, I believe that running inside of Node is triggering exceptions that do not occur in the browser. https://github.com/RedBow/pantry-for-good/blob/testing/client-core-components/client/modules/core/components/Footer.spec.js

Portions of both tests are also commented out, because the githooks refused to allow me to make work-in-progress commits.

redbow-kimee commented 6 years ago

these are the stack traces I've been getting 365 passing (1s) 2 failing

1) Footer Should have an organization: Invariant Violation: You should not use outside a at invariant (node_modules/invariant/invariant.js:40:15) at Link.render (node_modules/react-router-dom/Link.js:81:29) at node_modules/react-dom/lib/ReactCompositeComponent.js:793:21 at measureLifeCyclePerf (node_modules/react-dom/lib/ReactCompositeComponent.js:73:12) at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (node_modules/react-dom/lib/ReactCompositeComponent.js:792:25) at ReactCompositeComponentWrapper._renderValidatedComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:819:32) at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:359:30) at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:255:21) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at ReactDOMComponent.mountChildren (node_modules/react-dom/lib/ReactMultiChild.js:234:44) at ReactDOMComponent._createInitialChildren (node_modules/react-dom/lib/ReactDOMComponent.js:701:32) at ReactDOMComponent.mountComponent (node_modules/react-dom/lib/ReactDOMComponent.js:520:12) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at ReactDOMComponent.mountChildren (node_modules/react-dom/lib/ReactMultiChild.js:234:44) at ReactDOMComponent._createInitialChildren (node_modules/react-dom/lib/ReactDOMComponent.js:701:32) at ReactDOMComponent.mountComponent (node_modules/react-dom/lib/ReactDOMComponent.js:520:12) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:368:34) at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:255:21) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:368:34) at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:255:21) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:368:34) at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:255:21) at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:43:35) at mountComponentIntoNode (node_modules/react-dom/lib/ReactMount.js:102:32) at ReactReconcileTransaction.perform (node_modules/react-dom/lib/Transaction.js:141:20) at batchedMountComponentIntoNode (node_modules/react-dom/lib/ReactMount.js:124:15) at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-dom/lib/Transaction.js:141:20) at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:60:26) at Object.batchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:95:27) at Object._renderNewRootComponent (node_modules/react-dom/lib/ReactMount.js:317:18) at Object._renderSubtreeIntoContainer (node_modules/react-dom/lib/ReactMount.js:399:32) at Object.render (node_modules/react-dom/lib/ReactMount.js:420:23) at Object.renderIntoDocument (node_modules/react-dom/lib/ReactTestUtils.js:89:21) at renderWithOptions (node_modules/enzyme/build/react-compat.js:200:24) at new ReactWrapper (node_modules/enzyme/build/ReactWrapper.js:94:59) at mount (node_modules/enzyme/build/mount.js:19:10) at Context. (client/modules/core/components/Footer.spec.js:38:13)

2) Title should display a title: TypeError: Cannot read property 'pathname' of undefined at Title.componentDidUpdate (client/modules/core/components/Title.js:19:19) at measureLifeCyclePerf (node_modules/react-dom/lib/ReactCompositeComponent.js:73:12) at node_modules/react-dom/lib/ReactCompositeComponent.js:726:11 at CallbackQueue.notifyAll (node_modules/react-dom/lib/CallbackQueue.js:74:22) at ReactReconcileTransaction.close (node_modules/react-dom/lib/ReactReconcileTransaction.js:78:26) at ReactReconcileTransaction.closeAll (node_modules/react-dom/lib/Transaction.js:207:25) at ReactReconcileTransaction.perform (node_modules/react-dom/lib/Transaction.js:154:16) at ReactUpdatesFlushTransaction.perform (node_modules/react-dom/lib/Transaction.js:141:20) at ReactUpdatesFlushTransaction.perform (node_modules/react-dom/lib/ReactUpdates.js:87:32) at Object.flushBatchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:170:19) at ReactDefaultBatchingStrategyTransaction.closeAll (node_modules/react-dom/lib/Transaction.js:207:25) at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-dom/lib/Transaction.js:154:16) at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:60:26) at Object.enqueueUpdate (node_modules/react-dom/lib/ReactUpdates.js:198:22) at enqueueUpdate (node_modules/react-dom/lib/ReactUpdateQueue.js:22:16) at Object.enqueueForceUpdate (node_modules/react-dom/lib/ReactUpdateQueue.js:154:5) at WrapperComponent.ReactComponent.forceUpdate (node_modules/react/lib/ReactBaseClasses.js:83:16) at ReactWrapper. (node_modules/enzyme/build/ReactWrapper.js:245:27) at ReactWrapper.single (node_modules/enzyme/build/ReactWrapper.js:1421:25) at ReactWrapper.update (node_modules/enzyme/build/ReactWrapper.js:244:14) at Context. (client/modules/core/components/Title.spec.js:82:7)

jspaine commented 6 years ago

Hi, yeah enzyme can be a bit tricky sometimes. Calling debug() on a node and logging it is pretty helpful.

For the footer test, the error's because react-router expects Link's to be children of a Router. Because you're trying to mount the component on it's own it can't create the Link. You can just shallow mount it instead and pass the settings directly as a prop:

shallow(<Footer settings={{organization: "Foo"}}/>)

And then Foo is in a text node that's a child of the Link:

expect(f.find("Link").children().text()).to.have.string("Foo")

Also because both the bare component and the redux connected component are exported, and you only use the bare component there's no need to pass a mock redux store in context.

The Title component should be the same sort of thing, try and shallow mount it and pass the mock data and selector stubs as props. The selectors should be tested with the store so all you need to test here is that the component calls them with the right arguments.

redbow-kimee commented 6 years ago

Thanks. I think I figured it out. the attributes to the node in shallow() get injected into the component's props.

redbow-kimee commented 6 years ago

So strange thing happening now. I got both the footer and title to work, and then I pulled from staging to my branch because the git hooks were shouting at me. Now, when I run the title test, the componentDidUpdate method is never called when I call the update method. (Can proove this by adding a console.log statement) which is the item I'm trying to test. I did notice from the diff that other tests had been modified to use some sort of enzyme-adapter-15 (Something to do with newer version of react/enzyme?) and I used the same few lines of code. (my WIP commit of it is here, I got frustrated and used --no-verify: https://github.com/RedBow/pantry-for-good/blob/d66c051cf20f8547ee2cb0db8ba3b5acb5513ad3/client/modules/core/components/Title.spec.js)

jspaine commented 6 years ago

I think update doesn't do anything because the props haven't changed. If you do t.setProps({settings: {organization: "Foo"}}) then it updates.

The code in the Title components componentDidUpdate method should probably be run when the component is created as well as when it updates, so it actually does something with the inital props.

redbow-kimee commented 6 years ago

Okay. I got it all working now. see pr #391

tsukimi2 commented 6 years ago

Hi @jspaine, I'm new to the freeCodeCamp and I would like to help writing test cases for PantryForGood in order to get familiar with the codebase first before trying to help out with other issues. From the coverage run, it seems that there are still some files in server/lib folder that could still benefit from additional tests. Should I start working on those first and is somebody working on them already? Or are there other areas of the code that you would suggest to work on first? Please advise. Thanks.

jspaine commented 6 years ago

Hi, yeah that sounds good, let me know if you need anything.

tsukimi2 commented 6 years ago

Hi @jspaine, I've started unit testing on media-helper.js and notification-sender.js as previously mentioned. Would like to clarify the following 3 questions before doing a pull request for your review.

1) I see a folder server/tests/unit to place unit tests in, while I'm also seeing xxx-spec.js files within the same folder as their corresponding source files (e.g. lib/server/questionnaire-helper.spec.js. Which style of test files organization is preferable is this project (i.e. separate test folder or just place test files alongside their source files)?

2) When writing unit test for server/lib/notification-sender.js, I notice the following lines (lines 24-27) in function searchUserAndSetNotification:

        if (role === roleU){
          if(whoUpdated !== undefined && whoUpdated !== user._id) setNotification(user._id, notification, User)
          else setNotification(user._id, notification, User)
        }

Since both meeting either the if condition or the else condition are going call the same statement setNotification(user._id, notification, User), does the code really mean to say

        if (role === roleU){
          setNotification(user._id, notification, User)
        }

(i.e. can just combine the if and else statements into one) or was this part of the code originally intended to call different statements depending on whether the flow is to the if branch or else branch?

3) In server/lib/notification-sender.js function searchUserAndSetNotification line 20, is it ok to change the following code

export async function searchUserAndSetNotification(roleU, notification, whoUpdated) {
  await User.find({}, function(err, users) {
    if(err) throw err
    users.forEach(function(user) {
      user.roles.map(role => {
         .......................
      })
    })
  })
}

to the following equivalent code

export async function searchUserAndSetNotification(roleU, notification, whoUpdated) {
  const users = await User.find({})
    .catch(err => {
      throw err
    })

  users.forEach(function(user) {
    user.roles.map(role => {
       ........................
    })
  })
}

With the existing code, the await keyword in front of "await User.find(...)" in line 20 and async keyword in "export async function searchUserAndSetNotification" (line 19) seem redundant, since the User.find(...) is passing the query result to an anonymous callback anyway. Plus it seems easier to stub the User.find() when writing unit test for the function searchUserAndSetNotification of the second version. With the existing version, I couldn't find a way to stub the User.find() with its anonymous callback. What's your opinion on this?

jspaine commented 6 years ago

Hey, sounds good.

  1. I'd put the tests in the same folder as the files they're testing. We should move the one test in server/test/unit to avoid confusion.
  2. Yeah the if is pointless there, i thought there was supposed to be something to not send a notification to the user that made the change, but that code isn't going to work for that.
  3. It's better without the callback. The async/await in setNotification isn't needed either because nothing is waiting for it.
tsukimi2 commented 6 years ago

@jspaine I'll need some help in using git, since I've been using svn at work for the past couple of years and it's my first time using git.

I'm trying to create a pull request following the instructions on the Contribution Guide (https://github.com/tsukimi2/pantry-for-good/blob/staging/CONTRIBUTING.md) and got it working up to Step 6 of the section Creating a Pull Request where I did the commit successfully, but when I was trying to push my commits to my GitHub Fork I got the following error:

osiris@pantry-for-good:~/pantry-for-good$ git push -u origin feature/unit-test-server-lib Username for 'https://github.com': tsukimi2 Password for 'https://tsukimi2@github.com': remote: Permission to freeCodeCamp/pantry-for-good.git denied to tsukimi2. fatal: unable to access 'https://github.com/freeCodeCamp/pantry-for-good.git/': The requested URL returned error: 403

Could you give me some pointer as to what went wrong? Is it because I need to set the repos url of the origin to 'https://github.com/tsukimi2/pantry-for-good.git' instead? Currently the printout from my "git remote -v" is:

osiris@pantry-for-good:~/pantry-for-good$ git remote -v origin https://github.com/freeCodeCamp/pantry-for-good.git (fetch) origin https://github.com/freeCodeCamp/pantry-for-good.git (push) upstream https://github.com/freeCodeCamp/Pantry-for-Good.git (fetch) upstream https://github.com/freeCodeCamp/Pantry-for-Good.git (push)

Looking forward for your help.

jspaine commented 6 years ago

Yep, you can just git remote remove origin; git remote add origin https://github.com/tsukimi2/pantry-for-good.git

tsukimi2 commented 5 years ago

@jspaine In response to your suggestion "It's better without the callback. The async/await in setNotification isn't needed either because nothing is waiting for it.", while I agree that the async/await in setNotification should be removed since it's making setNotification a blocking operation for no apparent reason, currently it seems that some of the integration tests in server/tests/integration/customer.spec.js, user.spec.js, and volunteer.spec.js depend on the assumption of setNotification being a blocking operation. For example, lines 425-427 in customer.spec.js

      const volunteer = (await request.post('/api/auth/signin').send({email: 'volunteer@test.com', password: 'password'})).body
      const admin = (await request.post('/api/auth/signin').send({email: 'admin@test.com', password: 'password'})).body
      return expect(volunteer.notifications[0].message).to.equal(`Customer updated test was updated!`) && expect(admin.notifications[0].message).to.equal(`Customer customer test was created!`)

will throw an error when async/await is removed from function setNotification

  1) Customer Api
       notifications
         admin and volunteer notifications for a customer creation and update:
     TypeError: Cannot read property 'message' of undefined
      at Context.<anonymous> (server/tests/integration/customer.spec.js:427:48)
      at Generator.next (<anonymous>)
      at step (server/tests/integration/customer.spec.js:3213:191)
      at /home/osiris/pantry-for-good/server/tests/integration/customer.spec.js:3213:361
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:189:7)

because after the request.post, the above test code expects setNotification to complete setting the notification in db before proceeding to check the expect assertions.

In such cases, how should we modify the associated integration test cases to account for the async nature of function setNotification? Is it sufficient to use setTimeout to delay testing the expect statement after a slight delay (e.g. a second or so), or is there a better way to test this?

tsukimi2 commented 5 years ago

@jspaine I'm thinking about writing test cases for the schemas under the server/models folder. Or do you have other test cases that should be written with a higher priority?

jspaine commented 5 years ago

Ah good catch, it's definitely better to be able to wait for the route to respond than add a delay. There's a loop over the users to update that isn't awaited though, so it's a bit lucky the test works with the code as is.

The controllers have more logic and would be more worthwhile to test I think.

tsukimi2 commented 5 years ago

To enable route response to indicate whether notification has been set or not, should I modify the code something along the line as follow (i.e. (1) add a isUserNotify field to the request from client to indicate whether the server response needs to include whether notification has been set or not, (2) server waiting for the return of the result of setNotification or not depending on the value of isUserNotify, and (3) server response includes the field hasUserNotify when isUserNotify= true to indicate whether the result of setNotification.)? But to do this, it seems like there will be a number of places that originally call the functions searchUserAndSetNotification and searchVolunteerAndSetNotification to modify.

server/lib/notification-sender.js

export function extractUsersToNotify(user, filter) { return user.roles.filter(role => role === filter).reduce((acc, role) => user._id, null) }

export function extractVoluntersToNotify(volunteer, filter) { if(volunteer && Array.isArray(volunteer.customers) && volunteer.customers.length > 0) { return volunteer.customers.filter(customer => customer === filter).reduce((acc, customer) => volunteer._id, null) } else { return null } }

export function filterSetNotificationUsers(users, filter, extractFunc) { return users.map(user => { return extractFunc(user, filter) }).filter(elem => elem !== null) }


server/controllers/customer.js
- add isUserNotify to request body to indicate whether the server should wait for the result of setNotification to return or not.
- if isUserNotify = true, use promise to wait until all the setNotifications result has returned and then forward the result to server response as field hasNotifyUser (i.e. if isUserNotify = false, setNotifications will be performed asynchronously and the client code will continue on after calling setNotifications without waiting for the results of setNotifications to return, whereas if isUserNotify = true, setNotifications will be performed asynchronously but the client code will wait for setNotifications to return its results before continuing on.)
```javascript
  async update(req, res) {
    let hasNotifyUser = false
    const isUserNotify = req.body.isUserNotify === true ? true : false
    ...
    let newCustomer = await customer.save()
    ...

    // Sent Notification
    const filteredUsers = filterSetNotificationUsers(await User.find({}), 'roles/admin', extractUsersToNotify)
    const promise1 = filteredUsers.map(userId => setNotification(userId, {message:`Customer ${customer.fullName} was updated!`, url: `/customers/${customer._id}`}, User))

    const filteredVolunteers = filterSetNotificationUsers(await Volunteer.find({}), customer._id, extractVoluntersToNotify)
    const promise2 = filteredVolunteers.map(userId => setNotification(userId, {message:`Customer ${customer.fullName} was updated!`, url: `/customers/${customer._id}`}, User))

    updateFields(clientRoles.CUSTOMER, req.body.fields, customer._id)

    if(isUserNotify) {    
      Promise.all([promise1, promise2]).then(results => {  
        for(let i = 0; i < results.length; i++) {
          if(results[i].length !== 0) {           
            hasNotifyUser = true
            break
          }
        }

        res.json({  ...newCustomer._doc, hasNotifyUser })
      }) 
    } else {
      res.json({  ...newCustomer._doc, hasNotifyUser })
    }
  },
tsukimi2 commented 5 years ago

Ah good catch, it's definitely better to be able to wait for the route to respond than add a delay. There's a loop over the users to update that isn't awaited though, so it's a bit lucky the test works with the code as is.

The controllers have more logic and would be more worthwhile to test I think.

For testing controllers, would you mind if I install node-mocks-http to facilitate mocking the requests and responses, which are the dependencies of the controllers?

jspaine commented 5 years ago

I don't think the client should care if the notification has been set or not, if it fails for some reason it shouldn't prevent sending the response.

I'm not sure I see the point in node-mocks-http, can't you just define the request parameters and stub res.json?

tsukimi2 commented 5 years ago

I don't think the client should care if the notification has been set or not, if it fails for some reason it shouldn't prevent sending the response.

I'm not sure I see the point in node-mocks-http, can't you just define the request parameters and stub res.json?

Yes, you are correct. Doing that now.

By the way, I'm doing the unit testing on the customer controller now (server/controllers/customer.js). Mostly finished with the test code but have 3 questions to ask before submitting a pull request: 1) For the list function

  async list(req, res) {
    const customers = await Customer.find()
      .sort('-dateReceived')
      .populate('user', 'displayName')
      .populate('assignedTo', 'firstName lastName')

When I tried to stub the chained Customer.find().sort()... using

      const mockFindAll = {
        sort: function() {
          return this
        },
        populate: function() {
          return this
        }
      }

      const customerStub = sandbox.stub(Customer, 'find').returns(mockFindAll)

It gives the following error when the test is run: TypeError: _get__(...).find(...).sort(...).populate is not a function

I would like to ask whether I can append an exec() at the end of the chain in the js code, ie.:

  async list(req, res) {
    const customers = await Customer.find()
      .sort('-dateReceived')
      .populate('user', 'displayName')
      .populate('assignedTo', 'firstName lastName')
      .exec()

so that I can stub the Customer.find().sort.().populate()... chain using the following?

      const mockFindAll = {
        sort: function() {
          return this
        },
        populate: function() {
          return this
        },          
        exec: sinon.stub().returns(dummy_customers)
      }

      const customerStub = sandbox.stub(Customer, 'find').returns(mockFindAll)

This seems to work perfectly fine when I run the test case.

Or is there another better way to stub the chain in this case without needing to change the original js code?

2) When testing a function that has side effects, for example, the Customer.create(req, res) where besides the inputs (req, res) and output res.json(savedCustomer), you also have side effects like calling the searchUserAndSetNotification() function. When testing such functions, do we just need to assert that the output (res.json(savedCustomer)) conforms to what we expect, or do we need to also ensure that side effects like searchUserAndSetNotification() get called as well? (e.g. assert calledOnce on a stub on searchUserAndSetNotification())

3) For the function customerById() in server/controller/customer.js, I would like to test the path where the function throws a NotFoundError error, but when I try to do so with the following test code, it does not seem to work:

    it('should throw NotFoundError if customer find failed', async function() {
      const customerFindStub = sandbox.stub(Customer, 'findById').resolves(null)
      const ctrlSpy = sandbox.spy(customerCtrl, 'customerById')

      await customerCtrl.customerById(req, res, next, id)

      sinon.assert.threw(ctrlSpy, NotFoundError)
    })

After running the above test, it gives the error:

     NotFoundError: Not found
      at /home/osiris/pantry-for-good/server/controllers/customer.js:107:26
      at Generator.next (<anonymous>)
      at step (server/controllers/customer.js:1061:191)
      at /home/osiris/pantry-for-good/server/controllers/customer.js:1061:361
      at <anonymous>