markhuot / craft-pest

https://craft-pest.com
Other
38 stars 11 forks source link

Craft Commerce `Integrity constraint violation:` - State & session data leaking between tests. #102

Open joshuapease opened 1 year ago

joshuapease commented 1 year ago

Loving Craft Pest so much! This is an incredible plugin.

I’ve been evaluating using Pest instead of Codeception on a large Craft Commerce project.

While testing some functionality around the cart, I’m running into an Integrity constraint violation: exception.

As I dug into this particular exception, it lead me to some deeper questions about Pest.

But first… here’s the issue I hit (and a reduced test case in a repo).

Integrity constraint violation when saving the cart multiple times.

I’ve narrowed down the issue to Plugin::*getInstance*()->getCarts()->getCart(true) being called in more than one test. Some global state seems to be passed around between tests (those pesky singletons).

Here’s an example test that highlights the issue.

Also, here is a DDEV based repo that reproduces the issue.

https://github.com/joshuapease/craft-3-pest

<?php

use craft\commerce\Plugin;

it('gets the count of the cart', function(){
    $cart = Plugin::getInstance()->getCarts()->getCart(false);
    expect($cart->lineItems)->toBeEmpty();
});

// This happens even if this test was in a different Test.php file
it('get the cart while force saving', function() {
    // Force save throws the DB exception
    $cart = Plugin::getInstance()->getCarts()->getCart(true); 
    expect($cart->lineItems)->toBeEmpty();
});

Questions about module & session data being shared across tests.

After running XDebug, I noticed a few things.

When fist test is run gets the count of the cart

In craft\commerce\services\Carts::getCart()

I also inspected craft\commerce\services\Customers::getCustomer()

When second test is run get the cart while force saving

In craft\commerce\services\Carts::getCart()

image

In craft\commerce\services\Customers::getCustomer()

image

Should modules and session data be reset between tests?

I’ve noticed that Craft’s Codeception connector [seems to reset modules & plugins](https://github.com/craftcms/cms/blob/master/src/test/CraftConnector.php#L93-L115) between request.

Does Craft Pest do something similar?

russback commented 4 months ago

I am seeing something similar with a simple test to evaluate Pest.

First I have this template:

{% if currentUser %}
  <h1>{{ currentUser.email }}</h1>
{% else %}
  <h1>Not logged in</h1>
{% endif %}

I then have two tests, one with a user logged out, another with a user logged in. The expectation here is that the session is reset in each test:

it ('shows no user', function () {

    $text = $this
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe('Not logged in');

});

it ('shows user 1', function () {

    $user = 'foo@bar.com';

    $text = $this
        ->actingAs($user)
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe($user); 

});

What actually happens is this:

 Failed asserting that two strings are identical.

  at tests/DashboardTest.php:28
     24▕         ->assertOk()
     25▕         ->querySelector('h1')
     26▕         ->getText();
     27▕     
  ➜  28▕     expect($text)->toBe($user); 
     29▕     
     30▕ });
     31▕ 
     32▕ 

  --- Expected
  +++ Actual
  @@ @@
  -'foo@bar.com'
  +'Not logged in'

If I comment out either of the tests, the other runs as expected.

I also tried these approaches based on https://pestphp.com/docs/hooks but the result is the same:

beforeEach(function () {
    $this->tearDownActingAs();
});

afterEach(function () {
    $this->tearDownActingAs();
});

Moving the tests into their own file makes no difference either.

I tried calling the method explicitly in each test doesn't resolve this either, for example this, which should be the same as the afterEach hook:

it ('shows user 1', function () {

    $user = 'foo@bar.com';

    $text = $this
        ->actingAs($user)
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe($user);

    $this->tearDownActingAs();

});

And finally, clearing cookies has no effect either https://craft-pest.com/cookies.html#clearcookiecollection:

it ('shows user 1', function () {

    $this->clearCookieCollection();

    $user = 'foo@bar.com';

    $text = $this
        ->actingAs($user)
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe($user);

});

As we're just evaluating test frameworks, I'm not sure if this is the implementation by the plugin, or Pest underlying it.

Pest looks great and the plugin does a fantastic job with zero config setup, but this seems like a fundamental blocker.

If it is of any relevance, this site is running in a DDEV docker container, but I can't see how that would have a bearing on things.

markhuot commented 4 months ago

If you're on Craft 4 can you please try compose require markhuot/craft-pest-core and see if that makes any differences? If you're on Craft 5 please try composer require markhuot/craft-pest-core:dev-craft5. I believe the session/state issue is fixed over there. I'm a few weeks away from deprecating this repo and pointing everything over to core. It should be relatively stable over there!

russback commented 4 months ago

Thanks, Mark.

This is on 4, so I removed the plugin and all composer references and then ran composer require --dev markhuot/craft-pest-core:2.0.0-rc6

Running the same tests now looks like passes - great! - but I am getting these warnings:

WARN Your XML configuration validates against a deprecated schema. Migrate your XML configuration using "--migrate-configuration"!

Running this cleared that ./vendor/bin/pest --migrate-configuration

But then I get this on an individual test:

   WARN  Tests\AuthTest
  ! it shows no user → filemtime(): stat failed for /var/www/html/storage/runtime/cache/cc/CraftCMS--0213d27e-faa9-4719-ac8f-b630081142c2cc342e2b901bd877… 0.16s  
  ✓ it shows user 1 

Is that an issue with this repo? I'm running in ddev and everything else seems fine.

markhuot commented 4 months ago

Nope, unfortunately that's all "expected".

  1. craft-pest-core runs Pest 2 which is why the --migrate-configuration is necessary
  2. the filemtime() warning comes from Yii calling @filemtime() without a file_exists() first. It's a very real warning that would happen in production too if you had warnings enabled in production. I'm PR'ing Yii to try to get it resolved. As for Pest, the ! is a warning, but passing test, so your example above shows a passing test with one warning. Not idea, but the land we live in with Yii.
russback commented 4 months ago

Ah OK, thanks again.

I'm seeing one more failure but I'm guessing using a Factory for a User is relatively limited and that if we want to generate Users that have particular profiles or permissions we'd need to use a Seeder?

it ('shows user 2', function () {

    $user = \markhuot\craftpest\factories\User::factory();

    $text = $this
        ->actingAs($user)
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe($user->email);

});

I did try this but it failed on saveElement()

it ('shows user 2', function () {

    /** @var \craft\elements\User $user */
    $user = \markhuot\craftpest\factories\User::factory();
    $user->admin = true;
    Craft::$app->elements->saveElement($user);

    $text = $this
        ->actingAs($user)
        ->get('/test')
        ->assertOk()
        ->querySelector('h1')
        ->getText();

    expect($text)->toBe($user->email);

});
craft\services\Elements::saveElement(): Argument #1 ($element) must be of type craft\base\ElementInterface, markhuot\craftpest\factories\User given, called in /var/www/html/tests/AuthTest.php on line 35
markhuot commented 4 months ago

User factories will take any custom fields, same as any other element so you could do,

$user = \markhuot\craftpest\factories\User::factory()
  ->customField('value')
  ->email('foo@bar.com')
  ->create();

In addition, there's a helper for quickly creating an admin (since it's a common task) via,

$this->actingAsAdmin()->get('/test')->assertOk();

The specific error you're mentioning is because your passing a Pest factory in to the native ->saveElement and those two types are incompatible. If you swap the Craft native ->saveElement for the Pest ->create it should work,

$user = \markhuot\craftpest\factories\User::factory()
  ->admin(true)
  ->create();
russback commented 4 months ago

Thanks @markhuot all makes sense and looking good. And do users created in tests and seeding get removed automatically, or is there a need code in a teardown process at the end of each?

markhuot commented 4 months ago

No teardown. As long as you're using the RefreshesDatabase trait (which ships with the --init then the entire test will be run in a transaction. So any created users will be lost when the transaction automatically rolls back at the end of the test.

https://github.com/markhuot/craft-pest-core/blob/509e54f4d3c0b0560047176e3c23b7d1e1b115fd/stubs/init/Pest.php#L16