[Proposal] Add SQL Assertions for Tests

[DarkGhostHunter]

For what I could gather, there is no easy database assertions in Laravel testing suite.

It would be very handy to have assertions to count what queries where executed, and if a particular query was executed with bindings. Take for example these hypothetical test methods:

public function testQueryExecuted()
{
    $queries = DB::connection('sqlite')->capture(function() {
    
        User::create(['name' => 'John', 'email' => 'john@email.com']);
        $user = User::find(1);
        $user->name = 'Mike';
        $user->save();
        $user->delete();
    
    });
    
    $queries->assertQueryExecutedOn('users');
    $queries->assertQueryNotExecutedOn('users');
    
    $queries->assertInsertQueryExecutedOn('users', ['name' => 'Jhon'], false);
    $queries->assertSelectQueryExecutedOn('users', ['id' => '1'], false);
    $queries->assertUpdateQueryExecutedOn('users', ['name' => 'Mike']);
    $queries->assertDeleteQueryExecutedOn('users', ['id' => '1']);
    $queries->assertSoftDeleteQueryExecutedOn(User::class, ['id' => '1']);
    
    $queries->assertInsertQueryNotExecutedOn('users', ['name' => 'Jhon'], false);
    $queries->assertSelectQueryNotExecutedOn('users', ['id' => '1'], false);
    $queries->assertUpdateQueryNotExecutedOn('users', ['name' => 'Mike']);
    $queries->assertDeleteQueryNotExecutedOn('users', ['id' => '1']);
    $queries->assertSoftDeleteQueryNotExecutedOn(User::class, ['id' => '1']);

    $queries->assertNumberOfQueriesExecuted(4);
    $queries->assertNumberOfInsertQueriesExecuted(1);
    $queries->assertNumberOfSelectQueriesExecuted(1);
    $queries->assertNumberOfUpdateQueriesExecuted(1);
    $queries->assertNumberOfDeleteQueriesExecuted(1);
    $queries->assertNumberOfSoftDeleteQueriesExecuted(0);
    
    $queries->assertSqlQueryExecuted('select * from "user" where "id" = ?', [1]);
    $queries->assertSqlQueryNotExecuted('select * from "user" where "name" = ?', ['John']);
}

Unless there is already a package to allows these test.

There is an already DB::connection()->enableQueryLog() method that saves every query into an array, so including these kind of assertions shouldn't be hacky. Also, It would help immensely to internal Laravel Database testing.

[staudenmeir]

I would argue that it's better to test the result instead of the executed queries.

Instead of checking for an INSERT query:

$this->assertDatabaseHas('users', ['name' => 'John', 'email' => 'john@email.com']);

Instead of checking for an UPDATE query:

$this->assertDatabaseHas('users', ['id' => 1, 'name' => 'Mike']);

Instead of checking for a DELETE query:

$this->assertDatabaseMissing('users', ['id' => 1]);

Instead of checking for a SoftDeletes query:

$this->assertSoftDeleted('users', ['id' => 1]);

[mfn]

I find that the most practical use case of asserting the number of queries executed by your SUT is to find n+1 problems which can just too easily creep into complex systems without anyone noticing until it's too late.

[staudenmeir]

@mfn I agree. A testing integration of https://github.com/beyondcode/laravel-query-detector would be useful.

[DarkGhostHunter]

@staudenmeir Yes, but I would argue that archiving the result doesn't mean you optimized your queries. These new assertions would help archiving that. The Laravel Query Detector doesn't work on testing.

These assertions would be useful for Unit testing rather than Feature testing. I find this more useful since you could find easily creeping queries where real stuff happens.

[martinbean]

@DarkGhostHunter How do you envisage assertions like these…

$queries->assertSqlQueryNotExecuted('select * from "user" where "name" = ?', ['John']);

…working for databases that don’t use double quotes for quoting columns, i.e. MySQL?

[michaeldyrynda]

This is a good point, and would make testing with in-memory SQLite but using mysql in production much trickier.

Your tests would pass if you’re writing your assertion based on SQLite grammar but there’s no guarantee it works in mysql i.e. production.

The suggestion looks like mocking the database with an added layer of abstraction.

[DarkGhostHunter]

@DarkGhostHunter How do you envisage assertions like these…

$queries->assertSqlQueryNotExecuted('select * from "user" where "name" = ?', ['John']);

…working for databases that don’t use double quotes for quoting columns, i.e. MySQL?

Didn't know that catch. The inner method could normalize the SQL query to something standard across all database engines, like stripping double quotes. But in the meantime, that type of assertions could pushed down to another revision.

Back to the point, I think It should be fine with just asserting the number of queries processed and knowing in what table they were made.

It would be better if we could access the builder beforehand and check there what is going to be executed rather than picking the DB Query Logger, which it actually executes the statements.

[mfn]

Also, some queries will include actual IDs from the database. Unless you post-process the SQL, it's not going to work smoothly.

[DarkGhostHunter]

Then, where we can catch the query being sent, log it, and execute it?

[michaeldyrynda]

Why try and parse the query when you can observe the side effects in a much more simple fashion. Trying to parse the query builder is asking for brittle tests... and for things to break if the underlying framework changes.

[DarkGhostHunter]

Welp, the only thing left is to assert the number of queries executed in the database. And that's by counting the DB::getQueryLog() array.

It would be cool to have at least these assertions:

public function testQueryExecuted()
{
    $queries = $this->captureQueries($connection = 'sqlite', function() {
    
        User::create(['name' => 'John', 'email' => 'john@email.com']);
        $user = User::find(1);
        $user->name = 'Mike';
        $user->save();
        $user->delete();
    
    });
    
    $queries->assertQueriesExecuted(4);
    $queries->assertQueriesExecutedLessThan(5);
    $queries->assertQueriesExecutedMoreThan(3);
}

At least, using the number of queries, we can avoid queries creeping before it's too late.

[mfn]

Why try and parse the query when you can observe the side effects in a much more simple fashion.

This falls short on one of the useful goals: detect n+1 problems.

[michaeldyrynda]

So it’s less about what runs and more about how often it runs.

Marcel has a package that does this at runtime, I bet it could be adapted to count assertions.

[mpociot]

Hm yeah I think it would be possible to add some PHPUnit assertion helpers in my package.

You can already make use of the package in your tests. This is what one of the package internal tests looks like:

public function it_detects_n1_query_on_properties()
{
    Route::get('/', function (){
        $authors = Author::all();
        foreach ($authors as $author) {
            $author->profile;
        }
    });

    $this->get('/');

    $queries = app(QueryDetector::class)->getDetectedQueries();

    $this->assertCount(1, $queries);

    $this->assertSame(Author::count(), $queries[0]['count']);
    $this->assertSame(Author::class, $queries[0]['model']);
    $this->assertSame('profile', $queries[0]['relation']);
}

So this could be turned into something like this:

public function testN1()
{
    $this->get('/n1-issue');

    $this->assertHasNoN1Queries();
}

[ejunker]

Looks like it is possible with this package
https://github.com/mattiasgeniar/phpunit-query-count-assertions

[mfn]

Iff you are into asserting the SQL (some might argue you shouldn't)

My current recommendation is to:

• use https://github.com/spatie/phpunit-snapshot-assertions
• write a SQL driver
• add a method to sanitize the queries to be unified/void of IDs to guarantee reproducibility

You can use this one as a start (used only with Postgres):

        $actualQueries = trim(
            implode(
                "\n",
                array_map(
                // Replace any numeric literals with "fake" bind
                // placeholders. The framework recently optimized
                // whereIn queries to contain all-only integer
                // literals directly, which means it includes
                // IDs which may change during multiple test
                // runs, which we now manually need to normalize
                // Covers integers in `WHERE IN ()`
                // Covers simple `WHERE x =`
                    fn (QueryExecuted $query): string => preg_replace(
                            [
                                // Covers integers in `WHERE IN ()`
                                '/\d+(,|\))/',
                                // Covers simple `WHERE x = `
                                '/= \d+/',
                                // Covers simple `WHERE x =`
                                '/=\d+/',
                            ],
                            [
                                '?$1',
                                '= ?',
                                '=?',
                            ],
                            $query->sql
                        ) . ';',
                    $this->sqlQueryEvents
                )
            )
        );

I've been using SQL counting (not this package, custom implementation) for years before switching to snapshots, because it has serious drawbacks in practice:

• your SQL might still be incorrect or change (for the worse) and you might not realize it
  Have seen this in practice and actually uncovered bugs due to this
• you need to manually update the counts
  This was the biggest PITA: the bigger your project grows, the less you want to do this manually. We did it until a couple of thousand tests (not all of them changed all the time 😏 ) but it just gets tedious for not much benefit
  => snapshots are external and can be set up to auto-update in your local dev environment, which means in the end you just commit the changes to the SQL next to your code and it's within the review
• in a change of count, in the review unless the PR is trivial or the author explains the change, it's not obvious what changed
  => not an issue with snapshots

The drawbacks of snapshots:

• if your project grows and you refactor a lot, you get a LOT of changes SQL files no one is happy to review
• it might change out of the blue with a framework update
  => actually I consider this a feature: if you're considerate about the SQL, you want to know about this
• if you delete or rename tests, you might forget to clean up the snapshots and end up with stale snapshot files over time
  => there's no built-in solution to this and it just happens. Not the biggest problem but can be annoying.

HTH