thephpleague / factory-muffin

Enables the rapid creation of objects for testing
https://factory-muffin.thephpleague.com/
MIT License
531 stars 72 forks source link

exception with unique, acts to many times #343

Closed tzookb closed 9 years ago

tzookb commented 9 years ago

I had an overflow exception with the unique of "faker", I opend an issuse on their repo, but they showed me the problem is on the use with this package.

Please see the issue there: https://github.com/fzaninotto/Faker/issues/444

when I remove all the unique's from the configs this error doesn't happen.

GrahamCampbell commented 9 years ago

Hmmm. We're not doing anything special. You must be genuinely busting the unique threshold. Could I see your tests?

tzookb commented 9 years ago

I thought about it, but there aren't that many....

this are the configs for the muffin:

League\FactoryMuffin\Facade::define('Initial\Order\Models\Order', array(
        'reference' => 'unique:ean8',
        'address1' => 'secondaryAddress',
        'address2' => 'secondaryAddress',
        'comment_private' => 'realText|100;1',
        'company' => 1,
        'couponused' => 1,
        'transaction_id' => 1,
        'fname' => 'firstName',
        'lname' => 'lastName',
        'postcode' => 'postcode',
        'comment' => 'realText|100;1',
        'siteid_fk' => 'factory|Site',
        'countryid_FK' => 'factory|Country',
    ));

    League\FactoryMuffin\Facade::define('Initial\Order\Models\OrderPricing', array(
        'order_id' => 'factory|Initial\Order\Models\Order'
    ));

    League\FactoryMuffin\Facade::define('Initial\Order\Models\ShippingService', array(
        'name' => 'word',
        'shippingSupplier_id' => 'factory|Initial\Order\Models\ShippingSupplier',
    ));

    League\FactoryMuffin\Facade::define('Initial\Order\Models\ShippingSupplier', array(
        'name' => 'word',
        'taxPayable' => 0,
    ));

    League\FactoryMuffin\Facade::define('Initial\Order\Models\OrderLine', array(
        'orderid_FK' => 'factory|Initial\Order\Models\Order',
        'productid_FK' => 'factory|Product',
        'qty' => 'randomDigitNotNull',
        'price_user' => 'int',
        'price_supplier' => 'int',
        'cost' => 'int',
        'vat' => 'int',
        'shipping_extra' => 'int',
        'isFixedPoints' => 'int',
    ));

    League\FactoryMuffin\Facade::define('Initial\Storage\Models\StockOrder', array(
        'ipNumber' => 'unique:ean8',
        'product_id' => 'factory|Product',
        'closer_id' => 'int',

    ));

    League\FactoryMuffin\Facade::define('Product', array(
        'name' => 'unique:ean8',
        'sku' => 'unique:ean8',
        'stock_reordered' => 'int',
        'product_email' => 'email',
        'price_user' => 'int',
        'price_supplier' => 'int',
        'product_type' => 'string',
        'supplier' => 'string',
        'supplier_reference' => 'string',
        'created' => 'date',
        'productType_id' => 'int',
        'shippingScheme_id' => 'int',
        'virtual' => 0,
    ));

    League\FactoryMuffin\Facade::define('Bundle', array(
        'product_id' => 'factory|Product',
        'member_id' => 'factory|Product',
        'qty' => 'randomDigitNotNull',

    ));

    League\FactoryMuffin\Facade::define('Site', array(
        'name' => 'unique:ean8',
        'active' => 1,
        'url' => 'url',
        'shipping_schemeid_FK' => '1',
        'shipping_schemeid16_FK' => '1',
        'theme' => 'theme',
        'api' => 'url',
        'preordergroup' => '1',
        'type' => 'type',
        'pw_name' => 'word',
        'pw_address1' => 'word',
        'pw_address2' => 'word',
        'pw_website' => 'word',
        'pw_country' => 'word',
        'local_default' => 'en',
        'currency_default' => 'gb',
        'googleTracking' => '1',
        'order_status_id' => '1',
        'onHold' => '1',
    ));

    League\FactoryMuffin\Facade::define('Country', array(
        'country' => 'unique:country',
        'code_iso2' => 'unique:countryCode',
        'code_iso3' => 'xxx',
        'displayName' => 'word',
    ));

and my tests are only 14 of them... so even if there are 10 uniques generated for each test we are only at 140 uniques generated.....

I tested the faker unique with 10K and it was ok

<?php
require __DIR__ .'/../vendor/autoload.php';

$faker = Faker\Factory::create();
$faker->seed(5);

for ($i=0; $i < 10; $i++) {
   echo $faker->unique()->ean8, "\n";
}
GrahamCampbell commented 9 years ago

You could try putting echo 'hello'; or something in https://github.com/thephpleague/factory-muffin/blob/2.1/src/Generators/Generic.php#L51 to see how many times this method is actually called by factory muffin.

tzookb commented 9 years ago

I did a static counter as you said, and it is called 626 times, but this is not only uniques right? it is for all the definitions...?

thanks for your help, do you have any other suggestion?

**just checked now, the unique is called only 42 times.

GrahamCampbell commented 9 years ago

Then this can't an issue with factory muffin can it?

tzookb commented 9 years ago

but as I mentioned in the previous comments, when I test faker "unique" 10K times all is good, but when I run it with the muffin it gives this Magic error

scottrobertson commented 9 years ago

What code are you using to check 10k?

I am really not sure how this can be an issue with factory muffin :S we literally just call Faker, we do nothing specific with it.

tzookb commented 9 years ago

yes I browsed the muffin code....

thats is the reason Im frustrated :/

and here is the code for testing 10K

<?php
require __DIR__ .'/../vendor/autoload.php';

$faker = Faker\Factory::create();
$faker->seed(5);

for ($i=0; $i < 10000; $i++) {
   echo $faker->unique()->ean8, "\n";
}
tzookb commented 9 years ago

Sorry all, it seems it's not a problem here. This errors occurs only with countryCode and when it was used more than 14 times.

Thanks for your time and patience

scottrobertson commented 9 years ago

Good catch @tzookb :)

scottrobertson commented 9 years ago

Do you want to update Faker issue too so they know it's not an issue with us.

tzookb commented 9 years ago

thanks @scottrobertson , I just commented in the faker issue, I noticed that in the next version (currently it's committed) there will be much more countryCodes, in the version 1.4 there is only 13.

So I should wait until next version of faker will go out and than I would wait for the update of this package dependency for the new faker.

Just one question that bugs me, I have several tests and I understand that after I get to the limit of 13 countries codes generated I get the exception, but in each test there is a max of 2-4 countries generated, so this is happening only when all tests run together.

Doesn't the tests run each one separately? I mean every test resets all the system? because it looks like that faker status stays the same.....

scottrobertson commented 9 years ago

Doesn't the tests run each one separately? I mean every test resets all the system? because it looks like that faker status stays the same.....

No, the instance is persisted across all tests. Otherwise using unique would be kinda pointless for most people.

You could look into running phpunit in isolation, but to me it suggests your tests are written wrong.

tzookb commented 9 years ago
 /** @test */
public function had_error()
{
    //set world
    $order = League\FactoryMuffin\Facade::create('Initial\Order\Models\Order', [
            'status' => 'PENDING'
    ]);

    //here I would have one order in my sqlite DB with status PENDING
    //it creates in the BG Country and Product etc.

    //assert stuff       

}

/** @test */
public function order_returned()
{
    //set world
    $order = League\FactoryMuffin\Facade::create('Initial\Order\Models\Order', [
            'status' => 'WAREHOUSE'
    ]);

    //here I would have one order in my sqlite DB with status WAREHOUSE
    //I wouldn't want it to have 2 orders, as from the previous test, I want the DB clean

   //why should the faker save it's status?

    //assert stuff
}

Here is a sample 2 tests, why should the faker won't refresh? I would be happy to know how the tests should be properly written?

tzookb commented 9 years ago

is there an option to make the faker reset for each test?

scottrobertson commented 9 years ago

According to their docs:

// you can reset the unique modifier for all providers by passing true as first argument
$faker->unique($reset = true)->randomDigitNotNull; // will not throw OverflowException since unique() was reset

You could try the following in tearDown(), not sure if it will work though:

League\FactoryMuffin\Facade::getFaker()->unique(true);
tzookb commented 9 years ago

testing thanks!