Code Review Chaos to Control: Taming Test Quality

Code reviews are essential for maintaining high-quality software, but they often get bogged down by debates over coding style and team conventions. Instead of focusing on logic and functionality, engineers find themselves wrapped up in discussions about minor formatting details—draining both time and energy.

So how can we reduce these endless style debates without compromising quality? And more importantly, how can we establish consistent standards across teams and projects without adding unnecessary friction for developers?

Scenario: Lost Knowledge and Team Transitions

Consider a familiar scenario: D’Artagnan and the Three Musketeers are a well-coordinated team of QA engineers, collaboratively writing, reviewing, and updating tests daily. One day, due to organizational shifts such as project reprioritization, team expansions, or urgent production issues, the Three Musketeers are reassigned to different projects or support roles. Three new engineers join D’Artagnan, competent professionals but relatively unfamiliar with the team’s testing framework and established practices.

This change quickly highlights significant issues:

Knowledge Bottlenecks: D’Artagnan holds extensive undocumented knowledge, making it challenging and time-consuming to communicate best practices effectively. He ends up performing lengthy, repetitive code reviews.
Motivation Issues: The new engineers become demoralized by continuous corrections and extensive feedback, struggling to learn established conventions quickly.

These challenges raise essential questions about scalability, sustainability, and knowledge-sharing: How can a team minimize dependency on individual expertise, facilitate smoother onboarding, and maintain high code quality?

Creating a Rulebook: Clear and Accessible Standards

Recognizing the limitations of verbal knowledge sharing, we documented clear and practical standards.

Note: These guidelines extended beyond standard practices, such as PEP8 (already enforced by the flake8 linter), and addressed specific needs related to our QA processes.

Among the various rule categories we established, the most critical focus was on the structure of our tests. To ensure consistency, we adhered to a well-defined format when writing tests.

Since we used Vedro, an open-source Python framework for scenario-based testing, our rules were largely shaped by its syntax. Below is an example of a typical test scenario:


<span>import</span> <span>httpx</span>
<span>import</span> <span>vedro</span>
<span>from</span> <span>vedro</span> <span>import</span> <span>params</span>
<span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status</span><span>"</span>
    <span>@params</span><span>(</span><span>200</span><span>)</span>
    <span>@params</span><span>(</span><span>404</span><span>)</span>
    <span>def</span> <span>__init__</span><span>(</span><span>self</span><span>,</span> <span>status</span><span>):</span>
        <span>self</span><span>.</span><span>status</span> <span>=</span> <span>status</span>
    <span>def</span> <span>given_url</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>url</span> <span>=</span> <span>f</span><span>"</span><span>https://httpbin.org/status/</span><span>{</span><span>self</span><span>.</span><span>status</span><span>}</span><span>"</span>
    <span>def</span> <span>when_user_sends_request</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span> <span>=</span> <span>httpx</span><span>.</span><span>get</span><span>(</span><span>self</span><span>.</span><span>url</span><span>)</span>
    <span>def</span> <span>then_it_should_return_expected_status</span><span>(</span><span>self</span><span>):</span>
        <span>assert</span> <span>self</span><span>.</span><span>response</span><span>.</span><span>status_code</span> <span>==</span> <span>self</span><span>.</span><span>status</span>
<span>import</span> <span>httpx</span>
<span>import</span> <span>vedro</span>
<span>from</span> <span>vedro</span> <span>import</span> <span>params</span>

<span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status</span><span>"</span>

    <span>@params</span><span>(</span><span>200</span><span>)</span>
    <span>@params</span><span>(</span><span>404</span><span>)</span>
    <span>def</span> <span>__init__</span><span>(</span><span>self</span><span>,</span> <span>status</span><span>):</span>
        <span>self</span><span>.</span><span>status</span> <span>=</span> <span>status</span>

    <span>def</span> <span>given_url</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>url</span> <span>=</span> <span>f</span><span>"</span><span>https://httpbin.org/status/</span><span>{</span><span>self</span><span>.</span><span>status</span><span>}</span><span>"</span>

    <span>def</span> <span>when_user_sends_request</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span> <span>=</span> <span>httpx</span><span>.</span><span>get</span><span>(</span><span>self</span><span>.</span><span>url</span><span>)</span>

    <span>def</span> <span>then_it_should_return_expected_status</span><span>(</span><span>self</span><span>):</span>
        <span>assert</span> <span>self</span><span>.</span><span>response</span><span>.</span><span>status_code</span> <span>==</span> <span>self</span><span>.</span><span>status</span>
import httpx
import vedro
from vedro import params

class Scenario(vedro.Scenario):
    subject = "get status"

    @params(200)
    @params(404)
    def __init__(self, status):
        self.status = status

    def given_url(self):
        self.url = f"https://httpbin.org/status/{self.status}"

    def when_user_sends_request(self):
        self.response = httpx.get(self.url)

    def then_it_should_return_expected_status(self):
        assert self.response.status_code == self.status

Enter fullscreen mode Exit fullscreen mode

Some guidelines naturally followed Vedro’s best practices, such as clearly separating test steps (given, when, then). Other rules developed organically through our experience. For instance, we learned that debugging parametrized tests becomes significantly easier when each scenario includes a dynamic subject attribute, such as:


 <span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status {status}</span><span>"</span>
    <span>@params</span><span>(</span><span>200</span><span>)</span>
    <span>@params</span><span>(</span><span>404</span><span>)</span>
    <span>def</span> <span>__init__</span><span>(</span><span>self</span><span>,</span> <span>status</span><span>):</span>
        <span>self</span><span>.</span><span>status</span> <span>=</span> <span>status</span>
        <span>...</span>
 <span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status {status}</span><span>"</span>

    <span>@params</span><span>(</span><span>200</span><span>)</span>
    <span>@params</span><span>(</span><span>404</span><span>)</span>
    <span>def</span> <span>__init__</span><span>(</span><span>self</span><span>,</span> <span>status</span><span>):</span>
        <span>self</span><span>.</span><span>status</span> <span>=</span> <span>status</span>
        <span>...</span>
 class Scenario(vedro.Scenario):
    subject = "get status {status}"

    @params(200)
    @params(404)
    def __init__(self, status):
        self.status = status
        ...

Enter fullscreen mode Exit fullscreen mode

We also recognized the critical importance of explicit assertions within verification steps. Surprisingly, engineers sometimes omitted assertions due to oversight or fatigue, resulting in tests that falsely appeared successful without validating functionality.

Although clearly defined rules were beneficial, manual reviews were still labor-intensive and unreliable. Could automation resolve this issue?

Automating Rule Enforcement with Linters

Automated tests, like any other code, benefit from automated quality control. Leveraging linters for automated rule enforcement seemed logical. Since our team was already familiar with flake8, we explored extending it using tools like flake8-plugin-utils, simplifying the creation of custom plugins tailored to our specific needs.

How Linters Work: A Quick Overview

Linters analyze Python source code by parsing it into an Abstract Syntax Tree (AST) – a structured representation of code syntax. Every Python expression corresponds to AST nodes, which linters navigate to identify potential errors or violations.

Custom linter plugins typically consist of:

Errors: Unique codes and descriptive messages to highlight rule violations.
Visitors: Classes inheriting from ast.NodeVisitor used to traverse AST nodes.
Plugins: Classes that register visitors and integrate seamlessly with flake8.

A Simple Linter Rule in Action

Let’s say we want to check that our “then” step actually has an assert. A test violating this rule might look like:


<span>import</span> <span>httpx</span>
<span>import</span> <span>vedro</span>
<span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status</span><span>"</span>
    <span>def</span> <span>given_url</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>url</span> <span>=</span> <span>f</span><span>"</span><span>https://httpbin.org/status/200</span><span>"</span>
    <span>def</span> <span>when_user_sends_request</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span> <span>=</span> <span>httpx</span><span>.</span><span>get</span><span>(</span><span>self</span><span>.</span><span>url</span><span>)</span>
    <span>def</span> <span>then_it_should_return_expected_status</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span><span>.</span><span>status_code</span> <span>==</span> <span>self</span><span>.</span><span>status</span>   <span># <--- No assert here! </span>
<span>import</span> <span>httpx</span>
<span>import</span> <span>vedro</span>

<span>class</span> <span>Scenario</span><span>(</span><span>vedro</span><span>.</span><span>Scenario</span><span>):</span>
    <span>subject</span> <span>=</span> <span>"</span><span>get status</span><span>"</span>

    <span>def</span> <span>given_url</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>url</span> <span>=</span> <span>f</span><span>"</span><span>https://httpbin.org/status/200</span><span>"</span>

    <span>def</span> <span>when_user_sends_request</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span> <span>=</span> <span>httpx</span><span>.</span><span>get</span><span>(</span><span>self</span><span>.</span><span>url</span><span>)</span>

    <span>def</span> <span>then_it_should_return_expected_status</span><span>(</span><span>self</span><span>):</span>
        <span>self</span><span>.</span><span>response</span><span>.</span><span>status_code</span> <span>==</span> <span>self</span><span>.</span><span>status</span>   <span># <--- No assert here! </span>
import httpx
import vedro

class Scenario(vedro.Scenario):
    subject = "get status"

    def given_url(self):
        self.url = f"https://httpbin.org/status/200"

    def when_user_sends_request(self):
        self.response = httpx.get(self.url)

    def then_it_should_return_expected_status(self):
        self.response.status_code == self.status   # <--- No assert here!

Enter fullscreen mode Exit fullscreen mode

We define an error class:


<span>class</span> <span>StepAssertWithoutAssert</span><span>(</span><span>Error</span><span>):</span>
    <span>code</span> <span>=</span> <span>'</span><span>VDR001</span><span>'</span>  <span># VDR is our plugin's prefix </span>    <span>message</span> <span>=</span> <span>'</span><span>step </span><span>"</span><span>{step_name}</span><span>"</span><span> does not have an assert</span><span>'</span>
<span>class</span> <span>StepAssertWithoutAssert</span><span>(</span><span>Error</span><span>):</span>
    <span>code</span> <span>=</span> <span>'</span><span>VDR001</span><span>'</span>  <span># VDR is our plugin's prefix </span>    <span>message</span> <span>=</span> <span>'</span><span>step </span><span>"</span><span>{step_name}</span><span>"</span><span> does not have an assert</span><span>'</span>
class StepAssertWithoutAssert(Error):
    code = 'VDR001'  # VDR is our plugin's prefix     message = 'step "{step_name}" does not have an assert'

Enter fullscreen mode Exit fullscreen mode

Then we create our plugin:


<span>import</span> <span>ast</span>
<span>from</span> <span>flake8_plugin_utils</span> <span>import</span> <span>Plugin</span><span>,</span> <span>Visitor</span>
<span>from</span> <span>flake8_vedro.visitors</span> <span>import</span> <span>ScenarioVisitor</span>
<span>class</span> <span>VedroPlugin</span><span>(</span><span>Plugin</span><span>):</span>
    <span>name</span> <span>=</span> <span>'</span><span>flake8_vedro</span><span>'</span>
    <span>version</span> <span>=</span> <span>'</span><span>1.0.0</span><span>'</span>
    <span>visitors</span> <span>=</span> <span>[</span>
        <span>ScenarioVisitor</span><span>,</span>
    <span>]</span>
<span>import</span> <span>ast</span>
<span>from</span> <span>flake8_plugin_utils</span> <span>import</span> <span>Plugin</span><span>,</span> <span>Visitor</span>
<span>from</span> <span>flake8_vedro.visitors</span> <span>import</span> <span>ScenarioVisitor</span>

<span>class</span> <span>VedroPlugin</span><span>(</span><span>Plugin</span><span>):</span>
    <span>name</span> <span>=</span> <span>'</span><span>flake8_vedro</span><span>'</span>
    <span>version</span> <span>=</span> <span>'</span><span>1.0.0</span><span>'</span>
    <span>visitors</span> <span>=</span> <span>[</span>
        <span>ScenarioVisitor</span><span>,</span>
    <span>]</span>
import ast
from flake8_plugin_utils import Plugin, Visitor
from flake8_vedro.visitors import ScenarioVisitor

class VedroPlugin(Plugin):
    name = 'flake8_vedro'
    version = '1.0.0'
    visitors = [
        ScenarioVisitor,
    ]

Enter fullscreen mode Exit fullscreen mode

Because ast.NodeVisitor invokes visit_* methods based on the node type, we customize ScenarioVisitor to check classes (i.e., visit_ClassDef). We can omit certain details here for brevity, but conceptually, it might look like this:


<span>import</span> <span>ast</span>
<span>from</span> <span>typing</span> <span>import</span> <span>List</span>
<span>from</span> <span>flake8_plugin_utils</span> <span>import</span> <span>Visitor</span>
<span>class</span> <span>ScenarioVisitor</span><span>(</span><span>Visitor</span><span>):</span>
    <span>def</span> <span>visit_ClassDef</span><span>(</span><span>self</span><span>,</span> <span>node</span><span>:</span> <span>ast</span><span>.</span><span>ClassDef</span><span>):</span>
        <span>for</span> <span>step</span> <span>in</span> <span>get_assert_steps</span><span>(</span><span>node</span><span>):</span>
            <span>if</span> <span>not</span> <span>is_step_has_assert</span><span>(</span><span>step</span><span>):</span>
                <span>lineno</span><span>,</span> <span>col_offset</span> <span>=</span> <span>step</span><span>.</span><span>lineno</span><span>,</span> <span>step</span><span>.</span><span>col_offset</span>
                <span># Report error if no assert </span>                <span>self</span><span>.</span><span>error_from_node</span><span>(</span>
                    <span>StepAssertWithoutAssert</span><span>,</span>
                    <span>step</span><span>,</span>
                    <span>step_name</span><span>=</span><span>step</span><span>.</span><span>name</span>
                <span>)</span>
<span>import</span> <span>ast</span>
<span>from</span> <span>typing</span> <span>import</span> <span>List</span>
<span>from</span> <span>flake8_plugin_utils</span> <span>import</span> <span>Visitor</span>

<span>class</span> <span>ScenarioVisitor</span><span>(</span><span>Visitor</span><span>):</span>
    <span>def</span> <span>visit_ClassDef</span><span>(</span><span>self</span><span>,</span> <span>node</span><span>:</span> <span>ast</span><span>.</span><span>ClassDef</span><span>):</span>
        <span>for</span> <span>step</span> <span>in</span> <span>get_assert_steps</span><span>(</span><span>node</span><span>):</span>
            <span>if</span> <span>not</span> <span>is_step_has_assert</span><span>(</span><span>step</span><span>):</span>
                <span>lineno</span><span>,</span> <span>col_offset</span> <span>=</span> <span>step</span><span>.</span><span>lineno</span><span>,</span> <span>step</span><span>.</span><span>col_offset</span>
                <span># Report error if no assert </span>                <span>self</span><span>.</span><span>error_from_node</span><span>(</span>
                    <span>StepAssertWithoutAssert</span><span>,</span>
                    <span>step</span><span>,</span>
                    <span>step_name</span><span>=</span><span>step</span><span>.</span><span>name</span>
                <span>)</span>
import ast
from typing import List
from flake8_plugin_utils import Visitor

class ScenarioVisitor(Visitor):
    def visit_ClassDef(self, node: ast.ClassDef):
        for step in get_assert_steps(node):
            if not is_step_has_assert(step):
                lineno, col_offset = step.lineno, step.col_offset
                # Report error if no assert                 self.error_from_node(
                    StepAssertWithoutAssert,
                    step,
                    step_name=step.name
                )

Enter fullscreen mode Exit fullscreen mode

Where get_assert_steps(node) finds test steps that should contain assertions (e.g., those whose names start with then), and is_step_has_assert(step) checks whether there’s an assert statement in the body. To simplify the example, let’s assume that the assertions are only going to be done in then steps.


<span>def</span> <span>get_assert_steps</span><span>(</span><span>class_node</span><span>:</span> <span>ast</span><span>.</span><span>ClassDef</span><span>)</span> <span>-></span> <span>List</span><span>[</span><span>ast</span><span>.</span><span>FunctionDef</span><span>]:</span>
    <span>steps</span> <span>=</span> <span>[]</span>
    <span>for</span> <span>element</span> <span>in</span> <span>class_node</span><span>.</span><span>body</span><span>:</span>
        <span>if </span><span>(</span>
            <span>isinstance</span><span>(</span><span>element</span><span>,</span> <span>(</span><span>ast</span><span>.</span><span>FunctionDef</span><span>,</span> <span>ast</span><span>.</span><span>AsyncFunctionDef</span><span>))</span>
            <span>and</span> <span>element</span><span>.</span><span>name</span><span>.</span><span>startswith</span><span>(</span><span>'</span><span>then</span><span>'</span><span>)</span>
        <span>):</span>
            <span>steps</span><span>.</span><span>append</span><span>(</span><span>element</span><span>)</span>
    <span>return</span> <span>steps</span>
<span>def</span> <span>is_step_has_assert</span><span>(</span><span>step</span><span>:</span> <span>ast</span><span>.</span><span>FunctionDef</span><span>)</span> <span>-></span> <span>bool</span><span>:</span>
    <span>for</span> <span>line</span> <span>in</span> <span>step</span><span>.</span><span>body</span><span>:</span>
        <span>if</span> <span>isinstance</span><span>(</span><span>line</span><span>,</span> <span>ast</span><span>.</span><span>Assert</span><span>):</span>
            <span>return</span> <span>True</span>
        <span>if</span> <span>hasattr</span><span>(</span><span>line</span><span>,</span> <span>'</span><span>body</span><span>'</span><span>)</span> <span>and</span> <span>is_step_has_assert</span><span>(</span><span>line</span><span>):</span>
            <span>return</span> <span>True</span>
    <span>return</span> <span>False</span>
<span>def</span> <span>get_assert_steps</span><span>(</span><span>class_node</span><span>:</span> <span>ast</span><span>.</span><span>ClassDef</span><span>)</span> <span>-></span> <span>List</span><span>[</span><span>ast</span><span>.</span><span>FunctionDef</span><span>]:</span>
    <span>steps</span> <span>=</span> <span>[]</span>
    <span>for</span> <span>element</span> <span>in</span> <span>class_node</span><span>.</span><span>body</span><span>:</span>
        <span>if </span><span>(</span>
            <span>isinstance</span><span>(</span><span>element</span><span>,</span> <span>(</span><span>ast</span><span>.</span><span>FunctionDef</span><span>,</span> <span>ast</span><span>.</span><span>AsyncFunctionDef</span><span>))</span>
            <span>and</span> <span>element</span><span>.</span><span>name</span><span>.</span><span>startswith</span><span>(</span><span>'</span><span>then</span><span>'</span><span>)</span>
        <span>):</span>
            <span>steps</span><span>.</span><span>append</span><span>(</span><span>element</span><span>)</span>
    <span>return</span> <span>steps</span>

<span>def</span> <span>is_step_has_assert</span><span>(</span><span>step</span><span>:</span> <span>ast</span><span>.</span><span>FunctionDef</span><span>)</span> <span>-></span> <span>bool</span><span>:</span>
    <span>for</span> <span>line</span> <span>in</span> <span>step</span><span>.</span><span>body</span><span>:</span>
        <span>if</span> <span>isinstance</span><span>(</span><span>line</span><span>,</span> <span>ast</span><span>.</span><span>Assert</span><span>):</span>
            <span>return</span> <span>True</span>
        <span>if</span> <span>hasattr</span><span>(</span><span>line</span><span>,</span> <span>'</span><span>body</span><span>'</span><span>)</span> <span>and</span> <span>is_step_has_assert</span><span>(</span><span>line</span><span>):</span>
            <span>return</span> <span>True</span>
    <span>return</span> <span>False</span>
def get_assert_steps(class_node: ast.ClassDef) -> List[ast.FunctionDef]:
    steps = []
    for element in class_node.body:
        if (
            isinstance(element, (ast.FunctionDef, ast.AsyncFunctionDef))
            and element.name.startswith('then')
        ):
            steps.append(element)
    return steps

def is_step_has_assert(step: ast.FunctionDef) -> bool:
    for line in step.body:
        if isinstance(line, ast.Assert):
            return True
        if hasattr(line, 'body') and is_step_has_assert(line):
            return True
    return False

Enter fullscreen mode Exit fullscreen mode

And that’s it! We’ve successfully created a custom rule that enforces one of our team’s quality standards.

What We Achieved

With our best practices for writing tests now documented and automated:

Everyone has the same vision of what a properly structured test looks like, which reduces form/structure debates in code reviews.
Most style/structure checks are automated, meaning less time spent in review going over the same basic points again and again. As a result, tasks are delivered faster.
A universal standard can be applied to different teams and projects as long as they use the same framework. This drastically cuts down on repetitive “style” comments in code reviews across multiple codebases.

Where We Struggled

Introducing this custom linter to older, more “mature” projects sometimes triggered a large number of errors. We didn’t want to spend ages fixing everything at once, so we decided on a two-step approach:

Restrict rules for the entire test suite: In older code, we temporarily ignored the rules that generated hundreds of errors.
Strict rules for new or modified tests: For fresh code and any updates, we enforced the “strict” linter, ensuring best practices are upheld going forward.

This approach allowed us to keep legacy tests operational while gradually improving overall quality in each project.

What’s Next

We plan to keep improving our linter and expand coverage for our internal “set of rules”. By continuously enhancing these automated checks, we can maintain high-quality tests throughout our growing list of projects, keeping D’Artagnan and all the Musketeers (both new and old) happy and productive.

github.com/mytestopia/flake8-vedro

原文链接：Code Review Chaos to Control: Taming Test Quality

展开阅读全文

文章版权声明 1、本网站名称：拾光赋
2、本站永久网址：https://www.blogs.ink
3、本网站的文章部分内容可能来源于网络，仅供大家学习与参考，如有侵权，请联系站长QQ：805375623进行删除处理。
4、本站一切资源不代表本站立场，并不代表本站赞同其观点和对其真实性负责。
5、本站一律禁止以任何方式发布或转载任何违法的相关信息，访客发现请向站长举报
6、本站资源大多存储在云盘，如发现链接失效，请联系我们我们会第一时间更新。

THE END