yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Functional testing & $_FILES #4567

Closed dynasource closed 8 years ago

dynasource commented 10 years ago

@devs,

I'm having trouble with the functional testing of a upload function. There seems to be a problem with $_FILES not being loaded correctly in yii\web\UploadFile::loadFiles().

I guess its because the Request was already executed and filled. It is filling itself with a static property. A solution that works is adding 'empty(self::$_files)' but I am wondering if there is another solution (as the tests that will follow will fail eventually)?

Is there something in the Yii2 & Codeception integration that cleans the Request for files?

//possible solution
private static function loadFiles()
   if (self::$_files === null || empty(self::$_files)) {
//etc
   }
} 

//Codeception functional test
// uploadCept.php
$I = new dmn\yii2\modules\file\tests\codeception\TestGuy($scenario);
$I->amOnPage('index-test.php?r=file%2Ffile%2Fcreate');
$I->see('Create File');
$path = "//input[@type='file']";
$I->seeElement($path);
$I->attachFile($path, 'Tulips.jpg'); 
$I->click("//button[@class='btn btn-primary start']");
Ragazzo commented 10 years ago

Usually it is better to test such functionality with acceptance tests and WebDriver

dynasource commented 10 years ago

I don't think the advantages of the fast functional tests should be neglected.

Ragazzo commented 10 years ago

there are no advantages because php move_uploaded_file will not work in functional testing, so you cant correctly check things

dynasource commented 10 years ago

hey, you're right. I do see the file folder being generated but its empty. That means that functional testing the new file path won't be necessary any more. A pity.

Isnt there a workaround? A trick? I do see a filled $_FILES with a existing 'tmp_name'

dynasource commented 10 years ago

@developers

What about the following solution. Tested here and works.

class UploadedFile extends Object
    /../
    public function saveAs($file, $deleteTempFile = true)
    {
        if ($this->error == UPLOAD_ERR_OK) {

            if(YII_ENV==='test'){
                if ($deleteTempFile) {
                    return rename($this->tempName, $file);
                } elseif (is_file($this->tempName)) {
                    return copy($this->tempName, $file);
                }                
            }else{
                if ($deleteTempFile) {
                    return move_uploaded_file($this->tempName, $file);
                } elseif (is_uploaded_file($this->tempName)) {
                    return copy($this->tempName, $file);
                }                
            }

        }
        return false;
    }
Ragazzo commented 10 years ago

It is better to have separated class under yii2-codeception extension in this case

dynasource commented 10 years ago

using Yii::$classMap? ie. -> Yii::$classMap['yii\web\UploadedFile'] = '@vendor/yiisoft/yii2-codeception/UploadedFile.php';

Ragazzo commented 10 years ago

yes, that is one of solutions, maybe @qiangxue can suggest some other same way we also can skip testing check in Session class file

dynasource commented 10 years ago

the following code in tests/_bootstrap.php works nicely: \Yii::$classMap['yii\web\UploadedFile'] = DIR.'/_extensions/UploadedFile.php';

UploadedFile then requires 2 changes:

    /**
     * Saves the uploaded file.
     * Note that this method uses php's move_uploaded_file() method. If the target file `$file`
     * already exists, it will be overwritten.
     * @param string $file the file path used to save the uploaded file
     * @param boolean $deleteTempFile whether to delete the temporary file after saving.
     * If true, you will not be able to save the uploaded file again in the current request.
     * @return boolean true whether the file is saved successfully
     * @see error
     */
    public function saveAs($file, $deleteTempFile = true)
    {

        if ($this->error == UPLOAD_ERR_OK) {
            if ($deleteTempFile) {
                return rename($this->tempName, $file);
            } elseif (is_file($this->tempName)) {
                return copy($this->tempName, $file);
            } 
        }
        return false;
    }

    /**
     * Creates UploadedFile instances from $_FILE.
     * @return array the UploadedFile instances
     */
    private static function loadFiles()
    {
        //if (self::$_files === null) {
            self::$_files = [];
            if (isset($_FILES) && is_array($_FILES)) {
                foreach ($_FILES as $class => $info) {
                    self::loadFilesRecursive($class, $info['name'], $info['tmp_name'], $info['type'], $info['size'], $info['error']);
                }
            }
        //}
        return self::$_files;
    }

thanks for your time Ragazzo!

Ragazzo commented 10 years ago

@qiangxue what is your thoughts on this one about creating testing wrappers for Session and UploadedFile under yii2-codeception extension? will submit PR if needed

qiangxue commented 10 years ago

Sounds reasonable to me to have UploadedFile and other necessary wrapper classes in yii2-codeception (or perhaps under yii/test if they are not specific for codeception?)

Ragazzo commented 10 years ago

it depends on the way you think of specific, basically their usage is because of Codeception functional testing mode where we cant use headers, exit, move_uploaded_file, session_open and other functions. So i think maybe they should be moved to yii2-codeception rather then yii/test. However if using unit - tests with phpunit for example same problems will occur as you can note, maybe they are not so Codeception specific but console specific, so on what folder we should put them?

qiangxue commented 10 years ago

I think yii/test is better then. If your test framework doesn't have these problems, then just don't use these classes.

Ragazzo commented 10 years ago

Ok

Ragazzo commented 10 years ago

@qiangxue @samdark can one mark this as enh. since it was decided to implement separated classes for testing in console mode to avoid some problems with sessions and file uploads?

dynasource commented 9 years ago

@samdark: this issue can be closed when \yii\web\UploadedFile::reset() is placed in doRequest of https://github.com/Codeception/Codeception/blob/2.0/src/Codeception/Lib/Connector/Yii2.php

Ragazzo commented 9 years ago

@dynasource how this will solve problem since as i remember it was about move_uploaded_file and rename functions in php to handle this situation? You still will not be able to save file where you need it

dynasource commented 9 years ago

Perhabs I have been having another issue today When doing 2 functional tests in a row with attachFile didnt work correctly, because of the first file being remembered instead of being overridden by next tests. After including the reset in the Yii2 connector, the tests behaved correctly.

Ragazzo commented 9 years ago

I see , yes your issue is valid however it does not have anything related with this one . I think your fix is valid and can be applied in Yii2 Codeception module , can you submit PR in Codeception ?

dynasource commented 9 years ago

my mistake not looking thoroughly to my initial post. Sorry for that. I will look at it tomorrow.

yii-bot commented 8 years ago

Issue moved to https://github.com/yiisoft/yii2-codeception/issues/16

ilyachase commented 5 years ago

Sorry for necroposting but there is more nice and clean workaround: put this to the functional/_bootstrap.php file

namespace yii\web {
    function move_uploaded_file($from, $to) {
        return rename($from, $to);
    }
    function is_uploaded_file($file) {
        return true;
    }
}