Is disabling parallel testing a bad code design?

code review
1

Hello everyone,

I'm working on integration tests in my private API configuration. For this, I'm using actix_web for everything routing related and sqlx for database queries. I removed some things from the code I share (for privacy) so it's probably breaking but you get the idea.

My employee_test.rs look like this :

mod common;
use common::config::SITE_ID;
use chrono::NaiveDate;
use sqlx::PgPool;
use pretty_assertions::{assert_eq, assert_ne};
use backend::query::employees::{Employee, get_employee_info, add_new_employee, update_employee, get_all_employees};


pub fn create_test_employee(site_id: i32) -> Employee {
    Employee {
        id: None,
        first_name: "Test".to_string(),
        last_name: "Employee".to_string(),
    }
}

async fn cleanup_test_data(pool: &PgPool) -> Result<(), sqlx::Error> {
    sqlx::query("DELETE FROM employees").execute(pool).await?;
    Ok(())
}


/// Test cases for each function in employee. This test the basic of the function.

type TestResult = Result<(), Box<dyn std::error::Error>>;

#[actix_rt::test]
async fn test_add_and_get_one_employee() -> TestResult {
    let pool = common::config::setup_test_db().await;

    let new_employee  = create_test_employee(SITE_ID);
    let employee_id = add_new_employee(&pool, new_employee.clone()).await?;

    // Retrieve the employee we just added
    let retrieved_employee = get_employee_info(&pool, employee_id).await?;


    assert_eq!(retrieved_employee.id, Some(employee_id));
    assert_eq!(retrieved_employee.first_name, new_employee.first_name);
    assert_eq!(retrieved_employee.last_name, new_employee.last_name);

    cleanup_test_data(&pool).await?;
    Ok(())
}

#[actix_rt::test]
async fn test_get_all_employees() -> TestResult {
    let pool = common::config::setup_test_db().await;

    let employee_count = get_all_employees(&pool, SITE_ID).await?.iter().count();
    // Only one employee should be in the database because he was previously created and he isn't deleted.
    assert_eq!(employee_count, 0, "Expected 0 employee, got {}", employee_count);

    let new_employee  = create_test_employee(SITE_ID);
    add_new_employee(&pool, new_employee.clone()).await?;
    let new_employee_count = get_all_employees(&pool, SITE_ID).await?.iter().count();

    assert_eq!(new_employee_count, 1, "Expected 1 employee, got {}", new_employee_count);

    cleanup_test_data(&pool).await?;

    Ok(())
}

#[actix_rt::test]
async fn test_update_an_employee() -> TestResult {
    let pool = common::config::setup_test_db().await;

    //Add an employee. He's by default active.
    let new_employee  = create_test_employee(SITE_ID);
    let employee_id = add_new_employee(&pool, new_employee.clone()).await?;

    // Retrieve the employee we just added
    let mut retrieved_employee_to_modify = get_employee_info(&pool, employee_id).await?;

    assert_eq!(retrieved_employee_to_modify.id, Some(employee_id));
    assert_eq!(retrieved_employee_to_modify.first_name, new_employee.first_name);
    assert_eq!(retrieved_employee_to_modify.last_name, new_employee.last_name);
    
    //We modify only first and last name
    retrieved_employee_to_modify.first_name = "Newtestfirstname".to_string();
    retrieved_employee_to_modify.last_name = "Newtestlastname".to_string();

    update_employee(&pool, retrieved_employee_to_modify, employee_id).await?;

    let modified_employee = get_employee_info(&pool, employee_id).await?;

    assert_eq!(modified_employee.first_name, "Newtestfirstname", "Excepected Newtestfirstname for first name, got {}", modified_employee.first_name);
    assert_eq!(modified_employee.last_name, "Newtestlastname", "Excepected Newtestfirstname for last name, got {}", modified_employee.last_name);

    cleanup_test_data(&pool).await?;

    Ok(())
}

What I want to test is everything that I have in employee.rs :

use serde::{Deserialize, Serialize};
use sqlx::{PgPool, FromRow, Error};
use chrono::{Local, NaiveDate, NaiveDateTime};
use crate::helper::option_naive_date;

#[derive(Serialize,Deserialize,FromRow,Clone, Default)]
#[sqlx(default)]
pub struct Employee {
    //General information 
    pub id : Option<i32>,
    pub first_name: String,
    pub last_name: String,
}

#[derive(Debug, thiserror::Error)]
pub enum EmployeeError {
    #[error("Employee with id {0} not found")]
    NotFound(i32),
    #[error("Database error: {0}")]
    DatabaseError(#[from] Error),
}


/// # Returns
/// A list of employees if successful or a EmployeeError if something goes wrong
pub async fn get_all_employees(pool: &PgPool, site_id: i32) -> Result<Vec<Employee>, EmployeeError> {
    let employees = sqlx::query_as::<_, Employee>(
        r#"
        SELECT
            e.id as "id",
            e.first_name as "first_name",
            e.last_name as "last_name"
        FROM
            employees AS e
        LEFT JOIN 
            sites site ON e.site_id = site.id
        WHERE
            e.site_id = $1
        "#
    )
        .bind(site_id)
        .fetch_all(pool)
        .await;

    match employees {
        Ok(employees) => Ok(employees),
        Err(e) => Err(EmployeeError::DatabaseError(e)),
    }
}

/// # Returns
/// The employee object if successful or a EmployeeError if something goes wrong
pub async fn get_employee_info(pool: &PgPool, employee_id: i32) -> Result<Employee, EmployeeError> {
    let employee =  sqlx::query_as::<_, Employee>(
        r#"
        SELECT
            e.id as "id",
            e.first_name as "first_name",
            e.last_name as "last_name"
        FROM
            employees AS e
        WHERE
            e.id = $1
        "#
    )
        .bind(employee_id)
        .fetch_one(pool)
        .await;

    match employee {
        Ok(employees) => Ok(employees),
        Err(Error::RowNotFound) => Err(EmployeeError::NotFound(employee_id)),
        Err(e) => Err(EmployeeError::DatabaseError(e))
    }
}


/// # Returns
/// Ok if successful or a EmployeeError if something goes wrong
pub async fn add_new_employee(pool: &PgPool, employee: Employee) -> Result<i32, EmployeeError> {
    let id = sqlx::query_scalar(
        r#"
        INSERT INTO employees (
            first_name, last_name
        )
        VALUES (
            $1, $2
        )
        RETURNING id
        "#
    )
        .bind(employee.first_name)
        .bind(employee.last_name)
        .fetch_one(pool)
        .await;

    match id {
        Ok(id) => Ok(id),
        Err(e) => Err(EmployeeError::DatabaseError(e)),
    }
}


/// # Returns
/// Ok if successful or a EmployeeError if something goes wrong
pub async fn update_employee(pool: &PgPool, employee: Employee, employee_id: i32) -> Result<(), EmployeeError> {
    let result = sqlx::query(
        r#"
        UPDATE employees
        SET first_name = $1,
            last_name = $2
        WHERE id = $3
        "#
    )
        .bind(employee.first_name)
        .bind(employee.last_name)
        .bind(employee_id)
        .execute(pool)
        .await;

    match result {
        Ok(_) => Ok(()),
        Err(e) => Err(EmployeeError::DatabaseError(e)),
    }
}

As you can see, since it need a database connection, it falls into integration test otherwise I may have done some unit tests with mocking (I haven't done that because it will require a lot of code refactor).

So the issue is that one of the tests fails because they all start at the same time. I fixed the issue using the command :

cargo test -- --test-threads=1

But I read that it's bad using that. How can I improve my code then ?

Thank you.

2

The "proper" solution here is for each test to create a new database from scratch, so they do not share any state.

If that is infeasible but you want to make the tests not require --test-threads=1, you can use a static Mutex to make every test wait its turn before using the shared database.

7 Likes
3

Requiring --test-threads is problematic. It's inconvenient, non-standard, and can be confusing if the tests fail by default in the simple cargo test.

Tests also should be order-independent, but with single-threaded tests with shared state it's easy to accidentally make one test depend on results of another test, which makes them unreliable and impossible to run in isolation.

Test databases are a common problem, especially if it's expensive to set up a new one for every test. Unfortunately Cargo's test harness doesn't have setup/teardown and grouping to help with this, so static Mutex<()> is the way to go. There's a crate that adds syntax sugar for it:

4 Likes
4

The thing is, I use a docker container to build a test database, I'm not sure that it will be a good idea but I can try. I think I need to do some benchmarks at this point.

So if I understand correctly, the code isn't that bad, the issue is that database testing is a nightmare ? I was afraid that my test cases were wrongly written haha.

I think I will add this serial_test library but before I do so, can you show me an example of how I can implement the static Mutex option in my tests so I can understand ?

5

The code is okay. If it was some other global mutable state, like a global variable holding a HashMap or std::env, then it could be a bad design. However, an external database is supposed to be shared and mutable, so it's hard to expect something else.

static HOLD_YOUR_HORSES: Mutex<()> = Mutex::new();

#[test]
fn test1() {
   // watch out `let _ =` is a footgun
   let _lock = HOLD_YOUR_HORSES.lock().unwrap(); 
   // use the shared database here
}


#[test]
fn test2() {
   let _lock = HOLD_YOUR_HORSES.lock().unwrap();
   // use the shared database here
}

You could also put the database connection handle inside the Mutex<Option<Connection>> to force it to be used in all db-related tests.

1 Like
6

You're setting yourself up for painfully slow test cycles this way once you add more and more tests, even for tests that don't need this shared resource.

If your DB engine supports namespaces you could use those to isolate tests. If not then maybe you could snapshot the docker container and spin up multiple instances under different ports... not exactly lightweight but at least you wouldn't lose parallelism.

2 Likes
7

Rather than use a docker container to spin up the database for your tests, you can leverage the sqlx crate test functionality so that it can create isolated databases for you for your tests: test in sqlx - Rust

8

I haven't tried to isolate the docker container myself but while I was looking for a solution like this since your reply, I found a package called testcontainers - Rust.

I will try this library. If this doesn't fit in my project, I think I will use the serial_test one because it's working great.