Code in automated tests should also be tested… right?

A lot of developers avoid writing automated code tests, or at least writing proper ones. Having a basic rendering test for a component or a class, generated by your framework, doesn’t count as testing.

This is completely reasonable. Writing tests screws you up on so many levels:

So it isn’t surprising that only experienced devs do it properly. You need to feel the struggle to actually understand its benefits. And, you need to learn how to test the stuff that will bite your ass in the future.

There are different types of tests: unit, integration, acceptance, e2e, smoke, visual regression tests, etc. Every type of test introduces a new set of problems and requires a different perspective on the code being tested.

The biggest trap when writing tests is that you actually never know if the test is correct. You’re writing code that tests other code, which means you have even more opportunities to make mistakes.

So you need a system to test the test, right? I don’t actually have a solution for this, only a few pieces of advice to give.

The code in the test should be as trivial as possible. No if statements, no for loops, no logic—this is forbidden. Minimize the complexity wherever you can.

The test should also function as documentation, which means it should be readable by a human. Your Product Manager or QA engineer should be able to understand it.

Having a good testing framework is important. At Productive, we’ve developed our own set of abstractions over ember-test-helpers library, which is provided by Ember.

Most of the code in unit/integration tests should look the same.

A typical test consists of:

Being rigid about this structure is crucial. This will help you keep your tests tidy and readable. It will minimise the possibility of making mistakes, but it will not prevent you from falling into “the biggest trap”.

A great way to test the test is to change the original code the test is testing and then seeing if the tests are failing as they should. It sounds trivial, but I’m pretty sure you’re not doing it that often (at least I’m not ).

As soon as you see the green light in the tests, you feel happy and move on to the next thing— because you’re probably in a rush. And what if you tested the wrong thing?

Let’s take a look at the following example:

Pretty basic. If there are no items, the “items-count” label should not render. We’re testing the {{if}} statement in the template. You’ll see a green light in your CI/CD pipeline and move on happily, right?

Not so fast. Take a closer look at the test: the CSS selector is invalid. We’re missing the the dot in the $(’items-count’) call. So it’s completely wrong, but the test is still passing.  This is a common pitfall.

Whenever you write an assertion, make sure it’s failing before it passes. You can do that by commenting out (or adjusting) the code responsible for the logic you’re testing.

In this example, you would need to remove the {{#if @items.length}} statement in the template and check out if the test is failing. You would notice that the test isn’t failing, which would indicate that you wrote an invalid test.

This is how you test the test.

The idea of changing the codebase to validate how well the tests are written is not new. It’s called Mutation testing and there are testing libraries that do that automatically. A good mutation library should be able to handle the problem from the example above.

If you’re interested, check out this article on Javascript Mutation Testing from Olle Lauri Boström. He’s using Stryker for mutating his Javascript tests. To avoid diving into more detail, I’m just gonna drop a quick quote from the article:

You can also try the TDD approach. First write a failing test followed by the code that makes the test pass. This would work well in our example because the test would be passing from the start, which isn’t allowed by TDD.

TDD is a somewhat holistic and naive approach. It doesn’t always work out in practice, at least not in the “Frontend land”. But the idea is great, so take all the good parts from it.

Our recently joined colleague, Jean Petrić, wrote an excellent academic paper on a similar topic. With a few of his fellow researchers, he tried to answer: “How Good Are My Tests?”

They made a list of 15 testing principles that capture the essence of testing goals and best practices from a quality perspective. Go on and read the article, it’s a gem!

That’s all folks. Happy testin’

Recently, I stumbled upon an article on arkency—Disadvantages of pull requests.

I soon realized this was interesting stuff, worth sharing with my colleagues. Immediately I started writing a follow-up article, debating every claim made in the text.

We practice PR review workflow in Productive’s engineering team daily. This workflow helps us with:

Productive’s Engineering team builds one SaaS product and works on two massive codebases—the backend (API) and the frontend (web app).

Every process comes with overhead, and so does this one, but I’ll argue it’s worth it.

In this blog post, I’ll quote key points from the mentioned article and give my thoughts on the matter.

Now, let’s discuss the disadvantages of pull requests!

Yes, this is true. But it shouldn’t be that big of an issue if you practice a good distribution of work in your team.

Most products consist of many modules which are usually different screens or sections in the app. You should always divide work across those modules, in a way that one PR doesn’t interact with the other.

If your codebase is not that modularized, you can ensure a few good practices to avoid this issue:

The most common mistake developers do is putting too much work into one PR, making both the review and merge process harder than it should be.

More often than not, developers fall into this trap. They think something like “I’ll just refactor this while I’m already here” or “I need to complete the whole thing before sending it for review”. This leads to oversized PRs and a slow development process.

3 PRs with 100 lines are reviewed and merged faster than 1 PR of 300 lines.

Of course, not all changes are the same. If we have 100 lines of boilerplate code, which is usually framework scaffolding, there’s no complexity there. Tests take a lot of lines but should be easy to read. Code with fewer dependencies will always be easier to grasp too.

Basically, the more a developer is experienced and familiar with the codebase, the more lines he/she is allowed to put in one PR. Less experienced developers on the project should strive to make their PRs as thin as possible.

I couldn’t agree more. There’s nothing more exciting in programming than getting a great idea, implementing it fast, and testing it out instantly.

The original author’s claim is that PR reviews kill that short feedback loop which makes programming less fun. But if that’s true, then it also means that:

Again, it all revolves around making small and easily reviewable PRs, but we’ve already acknowledged that, so let’s move on.

If you do have a large PR waiting review, that doesn’t mean you can’t try it out in the wild. You could easily deploy the code to a staging environment, or even production, but on a separate domain so only you and your team can try out the new changes.

At Productive, for our main frontend app, we have an automated PR deployment setup. When you create a new PR on Github or push commits to an existing PR, a Github action is triggered that deploys the code to the “review” environment.

It takes the name of the git branch and deploys the frontend app to the subdomain with the same name. For example https://cool-new-stuff.review.productive.io.

Voilà, now you can try your PR changes in the live environment two minutes after you’ve created the PR.

Plus, there’s another Github action that posts a comment with a test link, so you don’t need to type it into the browser’s URL bar manually:

This is a frontend app example hosted as a static asset on a CDN, but you could do it similarly with backend apps.

There’s another mechanism for achieving a short feedback loop which is safe if done properly. With feature flags, you can ensure that your PR is not introducing any breaking changes and that new code is implemented in isolation from existing features.

Feature flags allow you to try the new changes in an isolated scope, without changing anything in the rest of the system. The scope can be either one user, one account, or just one API request. It’s a powerful mechanism that our team uses on a daily basis. It allows us to push new changes to the live environment rapidly, without worrying about breaking things.

I agree that PR reviews tend to be superficial, especially when there’s too much to review. That only means that the reviewer is doing their job poorly. You can recognize this situation when you see a complex PR that only has code-style comments like “Fix naming” or “Wrong indentation”, etc.

If the PR seems like it’s too complicated—why not pair-review instead?

Pair programming (not to be confused with pair-reviewing) is useful for mentoring junior developers, but it’s a waste of time in a lot of other cases. That’s because you’re not solving complex problems all the time.

A better way to do it is to pair-review the PR with the author after most of the boilerplate is done and proper research has been made. Then you only talk about the complex stuff and you can leave the author of the PR to explain their thought process in detail.

There’s one more thing here… Programmers usually get this “feeling” when something stinks about their solution. They should be honest and leave comments in the PR, in every place where they feel they could be doing something wrong. That greatly increases the reviewability of the PR.

These situations happen quite often with inexperienced reviewers. When commenting on the PR, you should tell the author if the comment is a merge-blocker or not, and never insist on details too much.

When I’m working with inexperienced developers, I’ll usually tell them exactly how to fix something, with an explanation. For non-blocking mistakes, I’ll just comment to watch out for that in the next PR. Usually, I’m the one to merge the PR, so they don’t have to wonder whether the PR is finished or not.

When reviewing experienced dev’s PR, I’ll tell them what I think about the problematic places in the code, but I’ll mostly approve the changes and leave them to the author to do the fixes the way they want to.

Not everything the reviewer says is always correct or the right thing to do. The author should be the one to decide on the comments—because it’s their work.

A lot of times it’s hard to explain the fix to the author and it would be more efficient just to implement the fix yourself. But merging the unfinished PR could be dangerous, so I’d never suggest that. You’d also be missing the opportunity to educate the original author about the problem.

Pull requests don’t have to be done by only one person. Why wouldn’t you, the reviewer, just pull the branch, implement, and push the fixes back upstream? That way the author gets notified about what you did and you both have an opportunity to discuss the issue.

That’s also an effective thing to do when there are a few simple fixes required on the PR. Let’s say you notice a typo somewhere in the PR. It’s much faster to fix it yourself than to leave a comment and wait for the author to do the fix. You could do that directly in Github and it would take like 30 seconds.

This is an interesting and important claim. Can your business afford to tear the live server down just to teach the developers some responsibility? Sometimes it’s worth it, but in most cases, you wouldn’t want to do that.

Code stability is not the only reason why we should be reviewing our PRs. There are also benefits of mentoring, knowledge sharing, better release organization, feature completeness, structured discussion around a diff, etc.

At Productive, we encourage people to self-merge PRs without a review when they’re confident that the changes are fine. The developer should have the freedom to do that in the part of the codebase where they have more ownership. I believe this is how the responsibility mindset can be trained. When people build something from the ground up, they will feel the responsibility to keep everything in great shape.

PRs will slow you down on refactoring. When you realize that some refactoring is needed, you need to switch to another branch to implement the refactoring separately, submit the PR to review, wait for the approval and merge the changes back into the original branch.

But it’s still better to do it that way, in my opinion. Refactoring PRs shouldn’t be complicated to review since there shouldn’t be any changes in the logic, only the code structure. That’s especially true if you have good test coverage. If you don’t, then maybe you should write the tests first?

Maybe the refactoring PR doesn’t need to go through the review process if everything is trivial. If it does, then mention to the reviewer that this is a refactoring-only PR that’s blocking feature development and the reviewer should prioritize the approval.

Having good team communication around PRs will minimize most of the downsides of the PR review process.

When it comes to pull request reviewing, the responsibility is both the author’s and reviewer’s. There’s one simple rule to follow:

Don’t be a prick when reviewing or authoring the PR.

The original article is basically saying that we should let developers do what they want because they might get offended by the review comments.

Obviously, that’s ridiculous. This doesn’t have anything to do with PR reviewing itself—it’s a communication problem. If your engineering team doesn’t know how to communicate, then you’ll have bigger issues than developers offended by a PR review.

Well, just do the migration separately on the main branch, before implementing the remaining logic in the PR. The world won’t collapse if you don’t always work on branches with PR reviews. You can always omit the process if you have a good reason for it.

The original article contains a list of suggestions on how to improve your team’s PR workflow. Those are all great ideas, which I encourage you to try. Thanks to arkency for writing a great and educational article! We’ve also published a copy of this article on Medium.

As with other organizational processes, we shouldn’t take them too seriously because they tend to generate overhead and slow work down. We should be pragmatic about PR reviews and when we notice they’re becoming a burden—we can skip them sometimes.

Don’t follow the rules blindly, try thinking with your head and do the right thing depending on the situation you’re in.

And a little remark for the end—let’s all be humble and respectful towards each other while reviewing each other’s code!

Last week I attended .debug, a developers conference, where my company held a booth.

The idea was to have a “change my mind” kind of setup, where we represent a radical idea, invite people to debate with us, and show them that we’re building some interesting stuff at Productive.

We decided to go with this one:

Jokes aside, React is a fine library. It’s important in web development because it introduced declarative and reactive templates, a paradigm shift that everyone needed at the time. There was a problem with rendering engines and reactivity back then (6 or 7 years ago) and React solved it pretty well.

As a side note, Ember solved the same problem earlier. It wasn’t as performant, though, and the framework was too opinionated to catch up with the way React had done it.

What happened after React gained popularity was a mess. It started a new trend in the community where everything revolves around hype, novelty, and creating new paradigm shifts. Every few months there were new libraries emerging, setting new standards of how we should write React web apps, yet solving problems that were, for the most part — already solved.

Let’s take “state management” as an example. Since React is missing a traditional dependency injection system (DI is achieved through component composition), the community had to solve this problem on its own. And it did. Over and over and again. Each new year brought a new set of standards.

React is just a rendering engine, and in a typical web app, you need many libraries to build a framework for a project — e.g. data layers, state management, routing, asset bundlers, and more.

The ecosystem behind React gave you too many choices of this sort, which fragmented the tech stack and caused the infamous “Javascript fatigue”.

One of the trends that also emerged was “framework comparison obsession”. JS frameworks were constantly compared with properties like rendering speed and memory footprint. This is irrelevant most of the time because a slow app is not caused by a slow JS framework, it’s caused by bad code.

As with every trend that is taking over the world — this one went too far, damaging new generations of web developers. I’m wondering how it’s possible for a library to be the most relevant skill on an average web developer’s CV? Even worse, it’s not even a library but a module inside that library. React hooks are more often mentioned as a “skill” as opposed to some actual skills like code refactoring or code review.

Seriously?! When did we stop bragging about the important stuff?

Why don’t you tell me, for example, that you know:

… not by mentioning the most starred library on Github, but by showing me one or two of your finest snippets.

… not by mentioning a popular state management library (preferably ending with “X”), but by telling me why “data should go down and actions should go up”. Or why state should be modified where it was created and not deeper in the component hierarchy.

… not by telling me that you know Jest or QUnit, but by explaining why it’s hard to automate end-to-end tests and why minimal meaningful rendering tests are 10% the effort and 90% the benefit.

… not by mentioning that you use CI/CD (as every other project today that has more than one person working on it), but by explaining that deployment and release should be separate so you should code new stuff in a way that doesn’t mess with the old stuff and can be turned on remotely.

… not by mentioning that you’re a “team player”, but by telling me that code review is just as hard on the reviewer’s side and that you know how to optimize your PRs for readability and clarity.

… because unless you’re a one-man band, you’ll hate your life if you don’t follow strict standards and conventions in a project. You should tell me that naming is hard and the broader the scope of the variable, the more time you should invest in coming up with a good name for it.

… because code review ensures product quality, reduces bugs and technical debt, builds common team knowledge, and more — but only if done thoroughly. Code review shouldn’t only be done top-down. It’s a great learning mechanism for less experienced team members.

… because it’s not about the GitHub stars, it’s about common principles that most of today’s JS frameworks share. Finding out about the pros and cons of other frameworks makes you understand your framework of choice better.

… because technology is only a tool for making products, not the process. Spending time on optimizing the process is always better than spending time on arguing about technology.

… because most of the time, optimization isn’t necessary at all.

… because pair-programming is, like code review, the most important practice for knowledge sharing and building team cohesion. It’s also fun!

… because every project has technical debt and you should stop whining about it and start refactoring. Every new feature should be preceded by minor code refactoring. Big refactoring or rewrites never turn out well.

So yeah, that’s why I think React ruined web development. People at the conference were intrigued by the claim and joined the debate eagerly. I had a great conversation with a few experienced React developers. Nobody agrees with the title of this article, saying “ruined” is too strong of a word. But most of them agree with the problems discussed in this article.

You can try to convince me that React isn’t that bad, and I will absolutely agree with you!

But instead, let’s debate about the more important topics — the work that we actually do as software engineers.