Ep.13: Finding Code Issues with Functional Testing
Teolin Codreanu
Software Developer | Microservices / APIs in Node.js, JavaScript, TypeScript, Nest.js / Koa / Express etc
This article is part of an in-depth comparison series of the top Node frameworks. We'll cover all the important aspects: market share, learning curve, ecosystem, security and more. To compare them properly, we will build the exact same app in all these frameworks (plus Vanilla Node), observe all the steps along the way and then benchmark them as we progressively add more functionalities.
Table of contents
In our previous episode, we created Postman requests to test the 13 routes we've built. They cover the "Happy Path" - if the user is doing exactly what we expect, the routes will work just fine.
However, I have yet to meet that mythical bunch of users who are doing exactly what they should do. Probably a better definition of a "user" would be "someone who's hell-bent on finding creative and innovative ways to mess up your app". I'll submit that definition to Webster's dictionary right away.
So, what if the user does something unexpected? Like calling a route that doesn't exist? Or a route that does, but with the wrong method? Or with the wrong ID? And so on. Well, we said we'll leave Exception Handling for another day, but some exceptions are built-in. We should at least cover those with tests, to make sure they fail as expected.
So, what do we already cover in our code? The following situations:
There are many other possible exceptions that we do not cover yet. For those we do, we want to make sure we do so correctly and in the same way for all the frameworks used.
404 errors
Functional tests for errors are pretty similar to those in the Happy Path. For example, when we look for the user with userId=X, we should receive a 404 error: "User not found", because we have no such user, we don't even have the right ID format. So the request and its test should look like this:
What about exceptions that we don't yet handle? Let's say we want to register a new user with POST /api/users/register, but we forget to send the body with the name, password etc along with the request. We should get a 400 error, but instead, we get a success message, 201, so our test will fail:
That is to be expected since we do not yet handle this exception. So, for now, let's modify the test to expect 201, and we will come back to this later on. Why not skip this altogether? Well, I do want to check that all the app variants have the same behavior so that we can compare them fairly. That being said, these are the 'unhappy path' tests that I want all my apps to pass in the exact same way:
The titles tell me at a glance what is the expected status code. You'll notice some have an exclamation mark and an alternate status code, like our earlier example. "200! missing body !400" tells me that it returns 200 instead of a more appropriate 400 code. For now.
You can find them all in the repo - there's a file called postman_collection.json.
...
All right, I ran all the tests on all 20+ variants we have so far and..., some failed. Which means the implementations are not exactly the same. Let's dig in!
Problem 1: Exception handling in the wrong place
Let's start with this test, which tries to find an inexistent user:
Instead of the expected 404 error, we get a 500 (server error), with the "Cannot convert undefined or null to object" message.
But why? Let's see the code that throws that:
It looks like we're retrieving a user, and if we don't find it, we set the 404 status and message. But do we really?
We get a 500 error status code, which is set in the catch block. That means our code never even reached the 'if(!user)' check, it breaks well before that.
The first issue with this code is that we do error handling in both the try and the catch blocks, which is generally a 'no-no'. It could be legit if we wrote some exception handling middleware, for example, but that's not the case. So, the first thing we need to do is move this line to the catch block. To do that, we need to make sure the UserService.retrieveUser() throws a 'Not Found' error, right now, it doesn't.
领英推荐
Problem 2: Unexpected type
Let's look into what's happening in UserService.retrieveUser():
We're getting the data from our fake database, then we're trying to find a user with userId==='X'. There is none, obviously, so line 136 will declare const user=undefined.
The trouble comes from our next line. We expected a User object, but we got undefined. TypeScript would have warned us (provided we added a User interface or type), but JavaScript is more permissive.
While it may have seemed like a good idea to delete the password property from a user before displaying the content of that user (you know, for security and such), trying to delete a property from 'undefined' will produce nothing but fireworks. So that's why our error message was "Cannot convert undefined or null to object". All right, so our code is flawed, let's fix that.
Problem 3: Errors not bubbling up
What we want is to throw an error when something other than the desired result occurs, and to catch it upstream, in the controller method, where we want to do the error handling.
Do we desire 'undefined' here? Well, I can't speak for you, but I certainly want a user to be a user object with specific properties, anything else will make me miserable for seconds on end. No user, no money:
if (!user) throw new Error('Not Found');
Oh, but we do have this check in the userController.retrieveUser(), you say:
if (!user) return res.status(404).json({ message: 'User not found' });
We do, but remember that we never reached that line, because the service class crashed before that. That's simply the wrong place to handle that error. Exceptions should be handled exactly where they occur and can be bubbled up if needed. In our case, it is: we don't just want to log an error to some log somewhere, we want to tell the person that we cannot find a user with this ID ('X'), and the proper way to relay that message is with a 404 Not Found HTTP Exception.
Problem 4: Unexpected object modifications
Another problem with this code is the way we hide the password - by deleting it from the user. We have no business modifying the entity, which may create other problems down the way. So, a better way is to use the rest operator to extract all the properties except for the password into another object and return that one. Our new UserService.retrieveUser() should be:
The 'else' branch could be expressed more concisely, at the expense of readability, but I don't advise that, unless you really hate your teammates. Here's how you could return the maskedUser directly and befuddle their karma:
else return (({ password, ...maskedUser }) => maskedUser)(user);
Bubbling up
Ok, now we move up, to the controller that calls this. We now know that the service will throw a 'Not Found' error when there's no user, so we will need to handle that case in the catch block, like so:
Great, thank you Functional Testing, I wouldn't have realised there's a problem without you. Well, for this extremely simple app with only one level of depth, any junior dev could have seen these problems quite easily. But code is never this simple in real life, and even a senior dev might miss some of these issues if it's several levels deep or distributed across many classes, files etc. This shows the importance of Test Driven Development (and we didn't even get to the good part, unit testing).
We have the same issues everywhere we are searching for a user, task or list by ID, so we should change all implementations accordingly.
There were some adjustments needed in the assertions, as the error message returned by Fastify differs slightly from the one returned by Express, so to catch both, we should expect something like this for a 404 error:
pm.expect(responseBody).to.satisfy(msg =>
msg.includes("Cannot GET /api/users/register") ||
msg.includes("Route GET:/api/users/register not found")
);
With all the changes in place, we can easily run all the tests with Newman on all the repos. Yay!