Some time ago, I stumbled upon an interesting technical test. However, the more I looked at the provided Rails app, the more issues I found with it, to the point that I find it good material for an educational (hopefully) piece. But before I start, two disclaimers:
- This is not a rebuttal of the test itself. And I’m certainly not trying to explain away my poor performance. The test was fair and fine; it provided realistic and functioning1 code, similar to a lot of things I’ve seen in actual applications. It just happens that the code, especially the specs, left room for improvement.
- The shortcomings in the code may be intentional. It is an assessment app, after all. Maybe its code is suboptimal because making it better wasn’t worth the effort. Or maybe the shortcomings are deliberate, to better assess the candidates. But that’s not important.
What matters is not really where the following code comes from; the important thing is that it is a good example of something that works but isn’t quite right. And making it right is important in real life. (And fun, too.)
The app and its tests (
You can find the repo here. To avoid spoiling the original test, I’ve changed the app description and the names of the models, controllers and other resources; I’ve also re-created the application instead of starting from the initial codebase. But apart from that, everything is the same, especially the tests.
And the tests are what I will focus on in this article. The application code doesn’t matter, and could be anything, as long as it makes the tests pass. The latter are made of two specs files, plus a support file:
/specs/api_spec.rbcontains specs for the two endpoints of the app’s API,
/specs/api_validation.rbcontains specs specific to the expected response when wrong parameters are sent to the API.
/specs/public_api.rbcontains a collection of helpers for the specs above.
(Obviously, the testing framework used here is RSpec.)
Overall, these specs work, in that they break if the implementation is wrong and pass when it is right (except for a couple), but I have several issues with them nonetheless. Poor naming, poor organization, lack of idioms… None of these issues is deal-breaking in itself, but taken all together, they are a bit much. If I was reviewing these specs in a pull request, for example, I’d ask for a series of fixes2:
1. That’s how you get ants (
RSpec is very polite and never demands you to do anything (I believe that it is because its original author is Canadian).
This is especially true when it comes to organizing your spec files: by default, RSpec only asks for files ending in
spec directory, and even this can be configured differently.
rspec-rails is a little bit more structuring and suggests a canonical directory structure, in which files are placed
according to the type of spec they contain:
/spec/models for model specs,
/spec/system for system specs, etc. Even
if this organization is not mandatory3, it has at least two benefits. Being conventional, it makes the specs
easier to contextualize and the suite easier to navigate. Also, it removes the need for some metadata, since RSpec can
infer them from the layout.
And so, we’ll start by moving the files around. Both
request specs: specs that use the whole
stack, making actual4 HTTP requests that go through the router, controllers, etc. (The only difference with system
specs is that the responses are not run by a browser, but asserted against as they are received.) The canonical place
for such specs is thus
/spec/requests/, and because the configuration option
on, we can remove the explicit declaration of the spec type by metadata, like so:
As for the helpers file, I would prefer to put it in a dedicated directory, too. Once again, this is not mandated by
RSpec, but by tradition support files go into a
/spec/support directory, so this is where we’ll
public_api.rb file. And while we’re at it, we’ll rename it
api_helpers, which is more fitting.
Having done so, we’d better rename the module defined within this file
ApiHelpers instead of
PublicApi. But instead
of updating both the
api_validation_spec.rb accordingly, we’ll update RSpec’s configuration.
Because the helper module is used by both specs groups, and both groups are made of request specs, we can configure RSpec
to always include the module in any request spec. This is done by adding a line in the
rails_helper.rb file, in
the configuration block:
However, for this module to be available, the file
support/api_helpers.rb must first be required. The easiest way to do so
is to tell RSpec to preemptively require all the files in the
/spec/support directory, which is as easy as uncommenting
a line in the generated
And now, instead of updating the
require and the
include instructions in the spec files, we can remove them
altogether. For example,
api_spec.rb now looks like this:
Functionally, we haven’t changed anything, but our
/spec directory is much more idiomatic now, and we’ve trimmed
a little bit of boilerplate from our spec files. Besides, keeping things neat and tidy prevents all kinds of bugs.
2. Phrasing! (
The main benefit of RSpec is arguably the ability to express your tests as specifications. I mean, it’s in the name! You
don’t have to give your tests meaningful titles, as if each one was the description of a notable part of the system,
but you lose half the value of RSpec if you don’t. Which is why I love to run the suite with the option
--format=documentation. When the specs are well named, you get a good understanding of the system; if they’re not, you see it right away.
Unfortunately, it is the case with the current output:
The first part, about the public API, is not too bad; at least it reads properly. The part on the API validation, though, tells very little. Or rather, it tells very little directly. Indirectly, it hints that these tests were probably an after-thought, and because they read more like the truncated descriptions of unit tests, rather than integration tests, they sound like code smell. (Yes, programming can lead to synesthesia.)
Without changing their code, let’s rename the specs so that the output reads better, and actually describes the behavior of the application, as a good specification document would.
When it comes to APIs, the best way to describe them is usually to go endpoint by endpoint. So let’s start by grouping the examples by the endpoint they cover:
A very small change, really, but it does improve the output:
Not only is this output closer to a decent specification, it also points out that our suite is not very well organized,
since two different endpoints are featured in the same example group. More importantly, it also highlights holes in our
test coverage: two endpoints are covered for validations, but only one when it comes to actual behavior. In other words:
when we read these specs, we learn nothing about how the
POST /beeps endpoint is supposed to work. And we have no
reason to believe that it is tested.
To fix the first of these issues, let’s move the examples around and replace the
with two new ones, one per endpoint:
Once again, we’ve just moved code around, but once again the output is improved:
We’ll deal with the absence (or not?) of tests for the
POST /beeps endpoint a bit later; for now, let’s get rid of
the meaningless “returns 400 status code” repetition. The tests behind this sentence are useful; the issue is
with their naming, because it doesn’t explain what “wrong” means in each case. But this is easy to change, and thanks
#specify alias for
#it, the code can still read nicely, too:
(Quick aside: note that I’ve kept a common group for the tests about validation, aptly named “validations”, even if it doesn’t add much to the complete specification. That is because, otherwise, the validations specs could end up mixed up with the other specs in the output when the suite is run in random order5, and that wouldn’t read well.)
Always work from the outside to the inside: now that everything is well organized and well labeled, we can start improving the code itself.
3. Danger Zone (
At its core, a test is made up of 4 phases: setup, execution, assertion and teardown. (The names may vary but the idea is always the same.) The setup and/or teardown phases are often skipped or hidden away, but a test without clear execution and assertion phases is a strong code smell. At best, it is probably a case of excessive abstraction; at worst, it is a case of excessive abstraction which hides an error in the test.
Looking at the specs for the Beeps API, this is exactly what we see: they have no setup, no teardown, but above all no
assertion phase. In code terms, there is no call to
At first, the specs may seem to work, because there is no action in the controller, which triggers an error, so the tests
are red. But even an rigged implementation of
BeepsController#save would be enough to make the specs pass, when they
We need to add proper assertions to ensure that we are actually testing what we claim to be testing:
Now the specs fail, and we can replace our slime with a proper implementation. (I won’t show any here, since we’re focusing on the tests.)
4. I swear to God, I had something for this. (
At this point, we’ve dealt with the strongest code smells or messiness. But I’d like to see if we can remove code, especially if we are to add some later. (After all, by tidying up, we’ve apparently revealed missing specs for the Beeps API). Helpers are good candidates for such trimming, because, as a codebase evolves, they can easily stop being used, and yet they’re rarely removed. Let’s look at those we have.
#post_beepmakes a call to the
#get_summarymakes a call to the
#summarymakes a call to the
GET /summariesendpoint, makes an assertion against the response, and parses the received payload.
#given_beepmakes a call to the
POST /beepsendpoint, and makes an assertion against the response.
The single responsibility principle doesn’t always apply to functions, but it’s still a good rule of thumb. On this
#given_beep are suspicious; let’s put them aside for later and consider the simpler
#get_summary helpers instead.
Interestingly, these methods do very little; but at the same time, what they do is rather significant, since calling the API is usually the heart of the “execution” phase of an API test. For these two reasons, I’d like to get rid of them: they don’t bring anything, but add a layer of indirection to our tests, and they do it in the exception phase, were clarity is the most important.
Because these helpers do so little, removing them is very easy:
To be fair, one could argue that the code that calls the Summaries API is a bit less legible than when going through
get_summary helper, because the parameters stand out less when put inside the URL. It is a rather subjective
tradeoff, and I could understand how a team would prefer the indirection of helper. However, I prefer to get rid of it.
Less code, less indirection, less chance of a bug.
5. The cumulative hangover will kill me. (
Now that we’ve dealt with the easy helpers, let’s look at the two suspicious ones – those that do more than one single
The problem with both is that, among their multiple responsibilities, they do assertions. It’s actually a double issue:
- Like with the expectations in the preceding helpers, the assertions are so central to a test that they shouldn’t be hidden away in a helper.
- These assertions are mixed with other things, are thus executed outside of the assertion phase.
You could argue that the 4-phases structure is too rigid, or that adding extra assertions when doing something is like a freebie, a bonus protection against regressions. I disagree, especially with the later. Having assertions outside of the assertion phase, or unrelated to whatever the test is about, makes the test harder to understand. And because they are so fundamental to the longevity of a system, tests need to be very easy to understand. That is true as soon as the test is run; a failure message unrelated to the test case is perplexing and breaks the flow. Compare for example these two failures on the same test:
In the first case, there is a direct relation between the expected result (“rounds presence hours”) and the error (“expected 7.83, got 7.833611111111111”); in the second, the failure is at the HTTP response level, not the rounding of a value.
#given_beep method is meant to be used to set up the test, preparing the necessary data in the setup phase. So
let’s remove anything that is not related to this. In fact, since we don’t want to test the API during this phase,
there is no need to even use it. We can create the data directly instead.
Following the same principle, the
#summary helper can be stripped of its call to
#expect. However, there is a
#summary is used in the execution phase, and as we’ve seen above, it can be better to
avoid hiding away the job done during this phase behind an indirection. Like with the
#get_summary helper, it’s hard
for me to be very categorical; both styles have advantages. However, one thing is certain: if my team opted to keep the
helper, I would require it to have a better name that explicitly conveys the idea that the endpoint is called and its
response parsed. So here would be the two options:
Personally, I chose to go with the former. (In part because I couldn’t find the helper’s name clear enough.)
6. Rampage! (
We’ve reorganized the tests (files included), removed cruft and boilerplate, and fixed conceptual errors in some of the tests. Before taking a step back and looking at our renovated suite, let’s clean up a bit:
#to_unixhelper is only used once now, so it can be removed and it’s relevant code used directly instead.
#given_beepis the only helper method left, and it only used in a single group, it can be moved there, and the whole support file can be deleted. (To be honest, we could also get rid of the helper altogether.)
- In the specs for the Summaries API, all the test use the same value for the
pokemon_idparameter, so none of the two
let(:second_pokemon) are of any use and can be deleted, too.
- Some of the specs for the Summaries API are about a portion only of the summary, so there is no need to make assertions against the whole JSON object.
- The canonical file structure of RSpec Rails allows the
/requestsdirectory to be named
/apiinstead, which is fitting here, so we’ll rename it.
In the end, we’re back to a vanilla RSpec configuration and a suite composed of two files, one for each endpoint:
Running the specs, we have a nice specification of the API:
Overall, I’d say that this we’ve done a pretty good job at turning something that just worked into something that is right. However, there is one last thing that we can do, and that is adding missing tests.
7. You’re not my supervisor! (
The specs for the Summaries API cover both its correct use (including edge cases) and its incorrect use, under the umbrella of “validations”. This is good. These specs are technically high-level integration tests, so I would advice to stick to testing the happy path, but because there are no low-level unit tests for the other cases, doing a bit more is fine. However, the specs for the Beeps API only cover the incorrect uses – no happy path, only the sad ones.
It wasn’t that bad so far because the happy path was sort of tested accidentally, through the misplaced assertion in
#given_beep helper. Now that we’ve remove this, there are no guardrails to prevent us from breaking the API
and not realizing it. But adding such a spec is very easy:
Apart from wrapping the validation specs in a
describe block for readability’s sake when running the suite in random
order, we’ve added a single test for the API’s happy path. Note the unusual look of this test; because we’re using
#change matcher, the code for the
execution phase is somehow sandwiched between assertions, but there are still two distinct phases.
This test is also notable for another thing: even though it is technically high-level, we’re making a low-level assertion, checking directly the database instead of going only through the API. I find this infringement of the Orthodoxy of the Test Pyramid justified here, since we have no lower-level tests anyway. YMMV.
And that’s a wrap! Polishing is fun and we could keep doing it (for example by introducing unit tests to reduce the surface area of the request specs), but good enough if fine, and in real life, our time would probably be best spent somewhere else. So let’s move on, and enjoy our descriptive, exhaustive and yet lean specs!
With one exception. ↩
Or kindly lead the PR author towards these fixes without directly asking for them. ↩
“RSpec Rails provides thoughtfully selected features to encourage good testing practices, but there’s no “right” way to do it. Ultimately, it’s up to you to decide how your test suite will be composed.”
Well, kinda. No data will go through the network, but the application will behave exactly as if. ↩
Which should always be the case, by the way. ↩