Added new property CDbTestCase.fixturesPath #2257

Closed
wants to merge 6 commits into
from

5 participants

@Ragazzo
Ragazzo commented Mar 24, 2013

I think this must be implemented, i have my test well-structured in different folders, so my fixtures folders looks like unit-tests folder (not equals of course). @cebe this one is also for issue #1936. Any thoughts of other core-developers?

@Ragazzo
Ragazzo commented Mar 24, 2013

I also think that if this will be implemented, then maybe add some else path to the condition and throw exception if path is not valid, this can save time for developers to not guessing why fixtures are not loaded,

@qiangxue qiangxue and 2 others commented on an outdated diff Mar 24, 2013
framework/test/CDbTestCase.php
@@ -39,6 +39,13 @@
*/
abstract class CDbTestCase extends CTestCase
{
+
+ /**
+ * @var string valid fixtures path for the current test. Path must be
+ * valid alias.
+ */
+ protected $fixturesPath;
@qiangxue
qiangxue Mar 24, 2013 Member

Suggest to make it public and name it as fixturePath (because fixture is used like an adjective here).

@Ragazzo
Ragazzo Mar 24, 2013

well, yes in my test-cases i made i public, but here fixtures property is protected, so i thought this one must be protected too. Ok, will update. Do we need to throw exception on not valid alias?

@samdark
samdark Mar 24, 2013 Member

Why not? That's a good safeguard.

@Ragazzo
Ragazzo Mar 24, 2013

well, ok, will add it and translations to russian to.

@qiangxue qiangxue commented on an outdated diff Mar 24, 2013
framework/test/CDbTestCase.php
@@ -113,6 +120,10 @@ public function getFixtureRecord($name,$alias)
protected function setUp()
{
parent::setUp();
+
+ if ($this->fixturesPath !== null)
@qiangxue
qiangxue Mar 24, 2013 Member

fixturePath

@qiangxue qiangxue commented on an outdated diff Mar 24, 2013
framework/test/CDbTestCase.php
@@ -113,6 +120,10 @@ public function getFixtureRecord($name,$alias)
protected function setUp()
{
parent::setUp();
+
+ if ($this->fixturesPath !== null)
+ $this->getFixtureManager()->basePath = Yii::getPathOfAlias($this->fixturesPath);
@qiangxue
qiangxue Mar 24, 2013 Member

fixturePath

@Ragazzo
Ragazzo commented Mar 24, 2013

Done. So i did not created new one translation, i've just picked up one that already exists.

@Ragazzo
Ragazzo commented Mar 26, 2013

guys is everything ok with this PR and it can be merged or i need to add smth., like tests or other?

@samdark
Member
samdark commented Mar 26, 2013

Looks good to me.

@klimov-paul klimov-paul and 1 other commented on an outdated diff Mar 26, 2013
framework/test/CDbTestCase.php
@@ -113,6 +120,17 @@ public function getFixtureRecord($name,$alias)
protected function setUp()
{
parent::setUp();
+
+ if ($this->fixturePath !== null)
+ {
+ if (Yii::getPathOfAlias($this->fixturePath))
+ $this->getFixtureManager()->basePath = Yii::getPathOfAlias($this->fixturePath);
@klimov-paul
klimov-paul Mar 26, 2013 Member

Could be refactored to:

if ($basePath=Yii::getPathOfAlias($this->fixturePath))
        $this->getFixtureManager()->basePath=$basePath;
@Ragazzo
Ragazzo Mar 26, 2013

not easy to read, but anyway as you command.

@klimov-paul klimov-paul and 2 others commented on an outdated diff Mar 26, 2013
framework/test/CDbTestCase.php
@@ -113,6 +120,17 @@ public function getFixtureRecord($name,$alias)
protected function setUp()
{
parent::setUp();
+
+ if ($this->fixturePath !== null)
+ {
+ if (Yii::getPathOfAlias($this->fixturePath))
+ $this->getFixtureManager()->basePath = Yii::getPathOfAlias($this->fixturePath);
+ else
+ throw new CException(Yii::t('yii','{path}" is not a valid directory.',array(
+ '{path}' => $this->fixturePath
@klimov-paul
klimov-paul Mar 26, 2013 Member

@Ragazzo, please maintain the code style: there are a lot of extra spaces in your code.

@Ragazzo
Ragazzo Mar 26, 2013

well, i thought that extra-spaces are just to easy code read. no?

@samdark
samdark Mar 26, 2013 Member

Yes and it will be used in Yii2. For now please refer to https://github.com/yiisoft/yii/wiki/Core-framework-code-style

@klimov-paul klimov-paul commented on an outdated diff Mar 26, 2013
framework/test/CDbTestCase.php
@@ -39,6 +39,13 @@
*/
abstract class CDbTestCase extends CTestCase
{
+
+ /**
+ * @var string valid fixtures path for the current test. Path must be
+ * valid alias.
@klimov-paul
klimov-paul Mar 26, 2013 Member

At the moment:

/**
 * @var string valid fixtures path for the current test. Path must be
 * valid alias.
*/

Better to be:

/**
 * @var string valid fixtures path for the current test. Path must be  valid alias.
 */
@Ragazzo
Ragazzo commented Mar 26, 2013

@klimov-paul added new php-doc info and removed extra spaces.

@klimov-paul klimov-paul commented on an outdated diff Mar 26, 2013
framework/test/CDbTestCase.php
@@ -113,6 +121,15 @@ public function getFixtureRecord($name,$alias)
protected function setUp()
{
parent::setUp();
+ if ($this->fixturePath!==null)
+ {
+ if ($basePath=Yii::getPathOfAlias($this->fixturePath))
+ $this->getFixtureManager()->basePath=$basePath;
+ else
+ throw new CException(Yii::t('yii','{path}" is not a valid directory.',array(
+ '{path}' => $this->fixturePath
@klimov-paul
klimov-paul Mar 26, 2013 Member

Still extra spaces around '=>'.

@klimov-paul klimov-paul commented on the diff Mar 26, 2013
framework/test/CDbTestCase.php
@@ -39,6 +39,14 @@
*/
abstract class CDbTestCase extends CTestCase
{
+
+ /**
+ * @var string valid fixtures path for the current test. Path must be
@klimov-paul
klimov-paul Mar 26, 2013 Member

Why new line? Explanation can be placed at the single line.
In this way it would be easier to read.

@Ragazzo
Ragazzo Mar 26, 2013

new line where? well, it is really strange to here that some extra spaces are not good for easy read and here new line is good for that))

@Ragazzo
Ragazzo commented Mar 26, 2013

Done. Fixed.

@klimov-paul
Member

Looks good.
@cebe, you were the most familiar with issue #1936, so I suggest the final decision should be yours.

@cebe cebe was assigned Mar 26, 2013
@cebe
Member
cebe commented Apr 24, 2013

Sorry guys but this would not work... Setting basePath of fixture manager after it has been initialized with init() does change the way it works. It may not call init scripts and cause unexpected behavior.
There should be one instance of CFixtureManager for a directory of fixture files.

Overriding getFixtureManager() in a test case is the best way to have different fixture direcories I think.

@cebe cebe closed this Apr 24, 2013
@cebe cebe referenced this pull request Apr 24, 2013
Closed

CDbTestCase fixtures format #1936

@Ragazzo Ragazzo deleted the unknown repository branch Apr 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment