Tests #130
Comments
|
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. |
|
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. |
|
@jspaine I'm interested in writing test cases for some the remaining modules. Please let me know for which i'm allowed try. Thanks! |
|
You are welcome to test anything you find that you think needs more testing. You can run the |
|
@kenjiO - I will be writing tests for questionnaire-view and section-view. QuestionnaireView.js Thanks. |
|
Hi, when I execute |
|
Hi, can you post the stacktrace, and whether it happens in client or server tests? ( |
|
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 npm ERR! A complete log of this run can be found in: npm ERR! A complete log of this run can be found in: |
|
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? |
|
@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. |
|
When I run
If I override the timeout for the Customer Api tests by adding I also experience timeouts when running the |
|
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? |
|
If you want to write some unit tests you can work on tests for the components in these directories client/modules/core/components I would look through some of the other tests and write yours in a similar way to keep things consistent. Thanks! |
|
sure thing. I can get started with those. |
|
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 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. |
|
these are the stack traces I've been getting
|
|
Hi, yeah enzyme can be a bit tricky sometimes. Calling For the footer test, the error's because react-router expects And then 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. |
|
Thanks. I think I figured it out. the attributes to the node in shallow() get injected into the component's props. |
|
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 |
|
I think update doesn't do anything because the props haven't changed. If you do 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. |
|
Okay. I got it all working now. see pr #391 |
|
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. |
|
Hi, yeah that sounds good, let me know if you need anything. |
|
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.
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 (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?
to the following equivalent code 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? |
|
Hey, sounds good.
|
|
@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 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 Looking forward for your help. |
|
Yep, you can just |
|
@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 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? |
|
@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? |
|
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. |
|
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 setNotification(id, notification, model){
return model.findOneAndUpdate(
{_id: id},
{$push: {notifications: notification}}
)
}
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
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 })
}
}, |
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? |
|
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:
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: 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?
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> |
Both the server and client portions of the application need more test coverage
The text was updated successfully, but these errors were encountered: