boboldehampsink / import

DEPRECATED - Import plugin for Craft CMS
MIT License
176 stars 28 forks source link

Make use of dataProviders in unit tests #166

Closed brammittendorff-dd closed 7 years ago

brammittendorff-dd commented 7 years ago

I see you are generating arrays to provide data, this can be done better in PHPUnit by a dataProvider:

https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers

boboldehampsink commented 7 years ago

I already use dataproviders everywhere, did you find a place where it's not used and should?

brammittendorff-dd commented 7 years ago

Yes thats right thats why i think you might have missed a few in file: https://github.com/boboldehampsink/import/blob/develop/tests/ImportServiceTest.php

You use $expectedData, which as you describe is data:

 $expectedData = array(
            array('row1value1', 'row1value2', 'row1value3', 'row1value4', 'row1value5'),
            array('row1value1', 'row2value2', 'row3value3', 'row4value4', 'row5value5'),
        );

And $data:

$data = array('settings1' => 'value1', 'settings2' => 'value2');
$data = array('settings1' => 'value1', 'settings2' => 'value2');

And you use a lot of arrays with variable name $settings i'm not quite sure if this is data or maybe should be private... But if it fits the requirements for the dataProvider then you can use it.

boboldehampsink commented 7 years ago

That's because that's the one expected result for that assertion. This has nothing to do with data providers.

brammittendorff-dd commented 7 years ago

So this isn't data you can provide?

boboldehampsink commented 7 years ago

You could, but we're only testing one outcome so that would be overhead.

brammittendorff-dd commented 7 years ago

The $data is 2 times all most the same array?

boboldehampsink commented 7 years ago

Yes, that's the content of the mocked csv. Please read the code first.

brammittendorff-dd commented 7 years ago

Again this isn't data you can provide?

boboldehampsink commented 7 years ago

We're testing only one multidimensional array - its totally unneeded. Now stop wasting my time.

brammittendorff-dd commented 7 years ago

Looks a bit sloppy to me, but hey it is your code. I'll leave you alone.