Does it make sense to test business functionality before doing a code review?

Question

Currently, we have a feature branch development environment. Features are specified as stories that each developer takes on. Once a feature is done on a feature branch, a pull request for said feature is opened. Now two steps are left: a) test business functionality: Somebody checks whether the feature was actually implemented as specified by the story. This should ideally be somebody else than the developer b) code review: Somebody does a code review on said branch. This should ideally be somebody else than the developer

a) has an impact on b): If the functionality is not implemented as desired, the code must definitely change.

b) has an impact on a): If there is a bug in the code, the feature might be impacted.

Based on this, it seems to be a good idea to first test the business functionality (a) and then do a code review (b).

Is this true? What are flaws in this setup?

Further question: If this is a way to go for it, what kind on tooling do you use to deploy many branches? Having microservices etc. will quickly give rise to conflicts (e.g., ports overlapping, server resource limitations, ...)

Answer 1

It seems rather nitpicky to determine if the order of reading code and performing any manual testing, especially since it's an iterative process.

Since it's not stated, there's an assumption that the developer who did the work didn't just write code and throw it over the wall. They tested it, by some combination of writing automated test code as well as running some manual tests over their changes. When the code review is initiated, the developer has reasonable confidence that what they are submitting is correct as far as they have understood the work before them and the review not only includes the product changes but all automated test code as well. If any manual testing was performed, the developer should be able to explain what testing was done.

If this holds, then I don't see why you need to draw a distinction between "test business functionality" and "code review". Perhaps don't think of it as a "code review", but rather a peer review of the work - the production code, the automated testing, and the manual testing performed. The reviewer is really checking the quality of all of these things, and perhaps doing some additional exploratory work to confirm that it is suitable. Depending on the nature of the changes, the effort can be put toward different things at the discretion of the reviewer.

Answer 2

Post-Hoc Testing is an Anti-Pattern

You have succinctly described the use case for test-first development. In general, you want to first determine if you've built the right thing. Then you need to determine if you've built the thing right!

With that said, the notion of divorcing functionality from code correctness is a false dichotomy. In order to meet a useful Definition of Done, you need to do both. More importantly, design and testing need to be done in close collaboration to avoid unnecessary hand-offs and process friction.

Treating unit and acceptance testing as post-hoc activities is often a hallmark of waterfall projects and "big, upfront planning." There are still some project domains where this is required or necessary, but if you're asking the question then your specific project probably isn't one of them.

Even if you aren't following an agile methodology per se, a well-crafted Definition of Done and a test-first approach are more likely to get you the business and technical results you need than trying to determine which post-hoc activity should precede the other. The generally-correct answer is that test design should come before coding or testing, so post-hoc anything (in whatever order) is likely to be an anti-pattern.

A Further Note on Code Reviews

When done properly, a code review is a form of validated learning. Teams that collaborate on code, or that use code discussions to capture lessons learned, are often more agile because it serves as a form of continuous improvement.

Reviewing pull requests, pair programming, or mob programming aren't really "code reviews" in the gating sense of the term. In my professional experience, teams that use code reviews for gating purposes are largely papering over a gap in their current processes or tooling, and should probably consider adding continuous integration (CI), automated testing, or a better branching model to their workflows. This is even more true when the reviews are about style or correctness. The former should be represent a working agreement within the team that's enforced by automated tools, while the latter is best addressed through executable tests.

In other words, if you're using manual code reviews as a form of acceptance testing, don't! There are more effective approaches, and the team should work together to pick the one that works best for the project.

Answer 3

I normally do a business test after the code has been reviewed and tested by the team. This to make sure that we deliver quality software to the users for testing. This prevents them from finding any obvious bugs and raises their trust in the delivered software.

But this does not mean that the users cannot be consulted during the development of the functionality. Showing work that is in progress and checking if you are on the right path is extremely valuable. This helps you build the right things. If it works as it should you can iterate on the right way.

The right thing is more important than the right way I think, as long as you do not willingly introduce technical debt. And how to that? That depends on the functionality and the team. The right way means (for me) that code is clean, documented where needed and optimized for at the time of writing. Building the right thing first can compromise any (an other) of those things. However if you know you have the right thing, you know the time needed to make it the right way, is not wasted