Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[ticket/10941] Unit tests for filespec class #857

Merged
merged 30 commits into from Jul 9, 2012

Conversation

Projects
None yet
6 participants
Contributor

Fyorl commented Jun 20, 2012

Ticket: http://tracker.phpbb.com/browse/PHPBB3-10941

Tests for current upload functionality and plupload functionality. To be merged prior to feature/attachment-improvements (phpbb#833).

@erikfrerejean erikfrerejean commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+ $this->init_filespec();
+ }
+
+ public static function additional_checks_variables()
+ {
+ $path = dirname(__FILE__) . '/../../phpBB/files/';
+ return array(
+ array($path . 'GIF', true),
+ array($path . 'JPG', false),
+ array($path . 'PNG', true),
+ array($path . 'TIF', false),
+ array($path . 'TXT', true),
+ );
+ }
+
+ public static function check_content_variables()
@erikfrerejean

erikfrerejean Jun 20, 2012

Using static for data providers is for phpunit < 3.3. As we expect 3.5 the static keyword should be dropped.
http://www.phpunit.de/manual/3.5/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers

@erikfrerejean erikfrerejean commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+ *
+ */
+
+require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php';
+require_once dirname(__FILE__) . '/../../phpBB/includes/utf/utf_tools.php';
+require_once dirname(__FILE__) . '/../../phpBB/includes/functions_upload.php';
+require_once dirname(__FILE__) . '/../mock/fileupload.php';
+
+class phpbb_filespec_test extends phpbb_test_case
+{
+ const TEST_COUNT = 100;
+ const PREFIX = 'phpbb_';
+ const MAX_STR_LEN = 50;
+
+ // Hexadecimal encoded images
+ public static $files = array(
@erikfrerejean

erikfrerejean Jun 20, 2012

static keyword can be dropped as the data providers shouldn't be static.

This pull request fails (merged b7eaf054 into ba21be8).

@erikfrerejean erikfrerejean and 2 others commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+ protected function tearDown()
+ {
+ $path = dirname(__FILE__) . '/../../phpBB/files/';
+ unlink($path . 'TXT');
+ foreach (phpbb_filespec_test::$files as $type => $data)
+ {
+ unlink($path . $type);
+ }
+ }
+
+ /**
+ * @dataProvider additional_checks_variables
+ */
+ public function test_additional_checks($filename, $expected)
+ {
+ global $user;
@erikfrerejean

erikfrerejean Jun 20, 2012

No need for the global as you create a new instance of that object in the test.

@Fyorl

Fyorl Jun 20, 2012

Contributor

This is here for the same reason as I mentioned about $config. One of the functions that is called by a method requires $user to be defined globally.

@p

p Jun 28, 2012

Contributor

In this case a comment should be added detailing which function requires this. And consider rewriting said function to not use the global.

You should also clear the global when the test finishes.

@erikfrerejean erikfrerejean and 1 other commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+ const MAX_STR_LEN = 50;
+
+ // Hexadecimal encoded images
+ public static $files = array(
+ 'GIF' => '47494638376101000100800000ffffffffffff2c00000000010001000002024401003b',
+ 'PNG' => '89504e470d0a1a0a0000000d4948445200000001000000010802000000907753de0000000c4944415408d763f8ffff3f0005fe02fedccc59e70000000049454e44ae426082',
+ 'TIF' => '49492a000c000000ffffff001000fe00040001000000000000000001030001000000010000000101030001000000010000000201030003000000d20000000301030001000000010000000601030001000000020000000d01020018000000d80000001101040001000000080000001201030001000000010000001501030001000000030000001601030001000000400000001701040001000000030000001a01050001000000f00000001b01050001000000f80000001c0103000100000001000000280103000100000002000000000000000800080008002f686f6d652f6b696d2f746d702f746966662e746966660000000048000000010000004800000001',
+ 'JPG' => 'ffd8ffe000104a46494600010101004800480000ffdb004300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdb004301ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffc20011080001000103012200021101031101ffc4001500010100000000000000000000000000000002ffc40014010100000000000000000000000000000000ffda000c03010002100310000001a07fffc40014100100000000000000000000000000000000ffda00080101000105027fffc40014110100000000000000000000000000000000ffda0008010301013f017fffc40014110100000000000000000000000000000000ffda0008010201013f017fffc40014100100000000000000000000000000000000ffda0008010100063f027fffc40014100100000000000000000000000000000000ffda0008010100013f217fffda000c0301000200030000001003ffc40014110100000000000000000000000000000000ffda0008010301013f107fffc40014110100000000000000000000000000000000ffda0008010201013f107fffc40014100100000000000000000000000000000000ffda0008010100013f107fffd9',
+ );
+
+ private $config;
+ private $filespec;
+
+ protected function setUp()
+ {
+ global $config;
@erikfrerejean

erikfrerejean Jun 20, 2012

Unneeded global, there is no global $config in the tests.

@Fyorl

Fyorl Jun 20, 2012

Contributor

Functions like unique_id() use the $config variable so I was under the impression that I needed to declare it global here and modify it appropriately so that unique_id() and others would work as expected.

@erikfrerejean

erikfrerejean Jun 20, 2012

In that case you are correct, didn't see the unique_id() call.

@erikfrerejean erikfrerejean commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
@@ -0,0 +1,215 @@
+<?php
+/**
+ *
+ * @package testing
+ * @copyright (c) 20012 phpBB Group

@erikfrerejean erikfrerejean and 1 other commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+ protected function setUp()
+ {
+ global $config;
+
+ if (!is_array($config))
+ {
+ $config = array();
+ }
+
+ $config['rand_seed'] = '';
+ $config['rand_seed_last_update'] = time() + 600;
+ $config['mime_triggers'] = 'body|head|html|img|plaintext|a href|pre|script|table|title';
+ $this->config = $config;
+
+ // Write some data to files
+ $path = dirname(__FILE__) . '/../../phpBB/files/';
@erikfrerejean

erikfrerejean Jun 20, 2012

Set $path as a class property so it can be reused. The value is the same everywhere and doesn't change so it doesn't really make sense to redefine it every time it is needed.

@naderman

naderman Jun 20, 2012

Owner

Also use DIR rather than dirname(FILE) as we require PHP 5.3 in 3.1.

This pull request fails (merged 2f27d531 into ba21be8).

@naderman naderman commented on an outdated diff Jun 20, 2012

tests/uploads/filespec_test.php
+require_once dirname(__FILE__) . '/../../phpBB/includes/functions_upload.php';
+require_once dirname(__FILE__) . '/../mock/fileupload.php';
+
+class phpbb_filespec_test extends phpbb_test_case
+{
+ const TEST_COUNT = 100;
+ const PREFIX = 'phpbb_';
+ const MAX_STR_LEN = 50;
+
+ // Hexadecimal encoded images
+ public static $files = array(
+ 'GIF' => '47494638376101000100800000ffffffffffff2c00000000010001000002024401003b',
+ 'PNG' => '89504e470d0a1a0a0000000d4948445200000001000000010802000000907753de0000000c4944415408d763f8ffff3f0005fe02fedccc59e70000000049454e44ae426082',
+ 'TIF' => '49492a000c000000ffffff001000fe00040001000000000000000001030001000000010000000101030001000000010000000201030003000000d20000000301030001000000010000000601030001000000020000000d01020018000000d80000001101040001000000080000001201030001000000010000001501030001000000030000001601030001000000400000001701040001000000030000001a01050001000000f00000001b01050001000000f80000001c0103000100000001000000280103000100000002000000000000000800080008002f686f6d652f6b696d2f746d702f746966662e746966660000000048000000010000004800000001',
+ 'JPG' => 'ffd8ffe000104a46494600010101004800480000ffdb004300ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdb004301ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffc20011080001000103012200021101031101ffc4001500010100000000000000000000000000000002ffc40014010100000000000000000000000000000000ffda000c03010002100310000001a07fffc40014100100000000000000000000000000000000ffda00080101000105027fffc40014110100000000000000000000000000000000ffda0008010301013f017fffc40014110100000000000000000000000000000000ffda0008010201013f017fffc40014100100000000000000000000000000000000ffda0008010100063f027fffc40014100100000000000000000000000000000000ffda0008010100013f217fffda000c0301000200030000001003ffc40014110100000000000000000000000000000000ffda0008010301013f107fffc40014110100000000000000000000000000000000ffda0008010201013f107fffc40014100100000000000000000000000000000000ffda0008010100013f107fffd9',
+ );
@naderman

naderman Jun 20, 2012

Owner

Please store these as actual image files in a fixture/ directory within this test directory and just read them from those files.

This pull request passes (merged d1a75dba into ba21be8).

This pull request passes (merged 187f7361 into ba21be8).

This pull request fails (merged 987e0393 into ba21be8).

This pull request passes (merged 6ee75282 into ba21be8).

This pull request passes (merged d3e570f0 into ba21be8).

This pull request passes (merged d8474a46 into ba21be8).

@p p commented on an outdated diff Jun 28, 2012

tests/uploads/filespec_test.php
+ $disallowed_content = explode('|', $this->config['mime_triggers']);
+ $this->init_filespec(array('tmp_name' => $this->path . $filename));
+ $this->assertEquals($expected, $this->filespec->check_content($disallowed_content));
+ }
+
+ public function test_clean_filename_real()
+ {
+ $available_chars = str_split('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ\'\\" /:*?<>|[];(){},#+=-_`');
+ $bad_chars = array("'", "\\", ' ', '/', ':', '*', '?', '"', '<', '>', '|');
+ for ($tests = 0; $tests < self::TEST_COUNT; $tests++)
+ {
+ $len = mt_rand(1, self::MAX_STR_LEN);
+ $str = '';
+ for ($j = 0; $j < $len; $j++)
+ {
+ $index = mt_rand(0, sizeof($available_chars) - 1);
@p

p Jun 28, 2012

Contributor

Tests should be deterministic. Use a fixed set of usernames with the required chars. If a certain combination is required, there is no guarantee that it will be tested on any particular run.

@p

p Jun 28, 2012

Contributor

This should also be done via the data provider interface, probably.

@p p commented on an outdated diff Jun 28, 2012

tests/uploads/fileupload_test.php
+ }
+
+ public function test_common_checks()
+ {
+ // Test 1: Valid file
+ $upload = new fileupload('', array('jpg'), 1000);
+ $file = $this->gen_valid_filespec();
+ $upload->common_checks($file);
+ $this->assertEquals(0, sizeof($file->error));
+
+ // Test 2: File too large
+ $upload = new fileupload('', array('jpg'), 100);
+ $file = $this->gen_valid_filespec();
+ $file->filesize = 1000;
+ $upload->common_checks($file);
+ $this->assertEquals(1, sizeof($file->error));
@p

p Jun 28, 2012

Contributor

You should check that the error is what you expect. The assertions are the same in this and two subsequent tests. If this cannot be done for whatever reason a comment should be added explaining the reason.

Contributor

p commented Jun 28, 2012

Lowercase fixture file names please. And I'm assuming they should gain appropriate extensions.

This pull request passes (merged af29ad88 into ba21be8).

@bantu bantu commented on an outdated diff Jul 2, 2012

tests/uploads/filespec_test.php
+ return array(
+ array('gif_copy', 'gif_moved', 'image/gif', 'gif', false, true),
+ array('non_existant', 'still_non_existant', 'text/plain', 'txt', true, false),
+ array('txt_copy', 'txt_as_img', 'image/jpg', 'txt', true, true),
+ array('txt_copy_2', 'txt_moved', 'text/plain', 'txt', false, true),
+ array('jpg_copy', 'jpg_moved', 'image/png', 'jpg', false, true),
+ array('png_copy', 'png_moved', 'image/png', 'jpg', true, true),
+ );
+ }
+
+ protected function tearDown()
+ {
+ $it = new DirectoryIterator($this->path);
+ foreach ($it as $fileinfo)
+ {
+ if (strlen($fileinfo->getFilename()) > 3)
@bantu

bantu Jul 2, 2012

Member

I would prefer an explicit list of files here instead of just deleting everything with a filename longer than 3 characters.

@bantu bantu and 1 other commented on an outdated diff Jul 2, 2012

tests/uploads/filespec_test.php
+ foreach ($bad_chars as $char)
+ {
+ $this->assertFalse(strpos($name, $char));
+ }
+ }
+
+ public function test_clean_filename_unique()
+ {
+ $filenames = array();
+ for ($tests = 0; $tests < self::TEST_COUNT; $tests++)
+ {
+ $this->init_filespec();
+ $this->filespec->clean_filename('unique', self::PREFIX);
+ $name = $this->filespec->realname;
+
+ $this->assertTrue(strlen($name) === 32 + strlen(self::PREFIX));
@bantu

bantu Jul 2, 2012

Member

assertSame

@p

p Jul 7, 2012

Contributor

For integer comparisons assertequal should probably be used.

This pull request fails (merged eba10c49 into ba21be8).

This pull request fails (merged 68a46456 into ba21be8).

This pull request fails (merged c3f0a466 into ba21be8).

@bantu bantu commented on an outdated diff Jul 3, 2012

tests/functional/fileupload_test.php
+
+ if (!is_array($config))
+ {
+ $config = array();
+ }
+
+ $config['rand_seed'] = '';
+ $config['rand_seed_last_update'] = time() + 600;
+
+ $user = new phpbb_mock_user();
+ $user->lang = new phpbb_mock_lang();
+
+ // Test 1: Invalid extension
+ $upload = new fileupload('', array('jpg'), 100);
+ $file = $upload->remote_upload('http://example.com/image.gif');
+ $this->assertEquals('URL_INVALID',$file->error[0]);
@bantu

bantu Jul 3, 2012

Member

space after comma

@bantu bantu commented on an outdated diff Jul 3, 2012

tests/uploads/filespec_test.php
+ }
+
+ /**
+ * @dataProvider additional_checks_variables
+ */
+ public function test_additional_checks($filename, $expected)
+ {
+ $upload = new phpbb_mock_fileupload();
+ $this->init_filespec(array('tmp_name', $this->path . $filename));
+ $this->filespec->upload = $upload;
+ $this->filespec->file_moved = true;
+ $this->filespec->filesize = $this->filespec->get_filesize($this->path . $filename);
+
+ $this->assertEquals($expected, $this->filespec->additional_checks());
+
+ $user = null;

@bantu bantu commented on an outdated diff Jul 3, 2012

tests/uploads/filespec_test.php
+ }
+ }
+ }
+
+ private function init_filespec($override = array())
+ {
+ // Initialise a blank filespec object for use with trivial methods
+ $upload_ary = array(
+ 'name' => '',
+ 'type' => '',
+ 'size' => '',
+ 'tmp_name' => '',
+ 'error' => '',
+ );
+
+ $this->filespec = new filespec(array_merge($upload_ary, $override), null);
@bantu

bantu Jul 3, 2012

Member

This method should probably return an instance of filespec instead of assigning it as a property of the test. It should therefore probably also be called get_filespec instead.

@bantu bantu commented on an outdated diff Jul 3, 2012

tests/uploads/filespec_test.php
+ $upload_ary = array(
+ 'name' => '',
+ 'type' => '',
+ 'size' => '',
+ 'tmp_name' => '',
+ 'error' => '',
+ );
+
+ $this->filespec = new filespec(array_merge($upload_ary, $override), null);
+ }
+
+ protected function tearDown()
+ {
+ global $user;
+
+ $files = array(
@bantu

bantu Jul 3, 2012

Member

I thought this list could be generated by the data providers, but apparently it is not that easy.

@bantu bantu commented on an outdated diff Jul 3, 2012

tests/uploads/filespec_test.php
+ $files = array(
+ 'gif_copy' => 1,
+ 'jpg_copy' => 1,
+ 'png_copy' => 1,
+ 'txt_copy' => 1,
+ 'txt_copy_2' => 1,
+ 'tif_copy' => 1,
+ 'gif_moved' => 1,
+ 'jpg_moved' => 1,
+ 'png_moved' => 1,
+ 'txt_as_img' => 1,
+ 'txt_moved' => 1,
+ );
+
+ $iterator = new DirectoryIterator($this->path);
+ foreach ($iterator as $fileinfo)
@bantu

bantu Jul 3, 2012

Member

You don't need to traverse the filesystem here, you can use $files and file_exists() instead now.

This pull request fails (merged 8f4b7a75 into ba21be8).

This pull request fails (merged 0c50f05c into ba21be8).

This pull request fails (merged e23ba6ac into ba21be8).

This pull request passes (merged 9e042b16 into ba21be8).

This pull request fails (merged 3ebba7ac into ba21be8).

@bantu bantu commented on an outdated diff Jul 6, 2012

tests/functional/fileupload_test.php
+ * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+ *
+ */
+
+/**
+ * @group functional
+ */
+class phpbb_functional_fileupload_test extends phpbb_functional_test_case
+{
+ public function test_form_upload()
+ {
+ // This test is marked as incomplete due to an apparent bug in the
+ // symfony framework which causes it to lose the mimetype of any file
+ // uploaded. Since filespec::is_image() relies on the mimetype, all
+ // image uploads fail. filespec::is_image() is fixed in:
+ // https://github.com/phpbb/phpbb3/pull/833
@bantu

bantu Jul 6, 2012

Member

The is_image change has been merged into develop. So this should be reflected in this pull request. I.e. it should not be marked incomplete anymore.

@bantu bantu commented on an outdated diff Jul 6, 2012

tests/functional/fileupload_test.php
+class phpbb_functional_fileupload_test extends phpbb_functional_test_case
+{
+ public function test_form_upload()
+ {
+ // This test is marked as incomplete due to an apparent bug in the
+ // symfony framework which causes it to lose the mimetype of any file
+ // uploaded. Since filespec::is_image() relies on the mimetype, all
+ // image uploads fail. filespec::is_image() is fixed in:
+ // https://github.com/phpbb/phpbb3/pull/833
+ $this->markTestIncomplete();
+
+ $path = __DIR__ . '/fixtures/files/';
+ $this->add_lang('posting');
+ $this->login();
+
+ // Test 1: Invalid extension
@bantu

bantu Jul 6, 2012

Member

Can these tests be (easily) moved into their own methods?

@bantu

bantu Jul 6, 2012

Member

The two existing methods should probably be classes instead. The requirements could then be setup in the constructor and you could have one method per test.

@bantu bantu commented on an outdated diff Jul 7, 2012

tests/uploads/fileupload_test.php
+ $filespec->height = 2;
+
+ return $filespec;
+ }
+
+ protected function tearDown()
+ {
+ // Clear globals
+ global $config, $user;
+ $config = array();
+ $user = null;
+ }
+
+ public function test_common_checks()
+ {
+ // Test 1: Valid file
@bantu

bantu Jul 7, 2012

Member

Here, tests can be trivially put into their own method. You can use the method name to describe what the test does, e.g. test_common_checks_valid_file

@p p commented on an outdated diff Jul 7, 2012

tests/mock/fileupload.php
+
+/**
+ * Mock fileupload class with some basic values to help with testing the
+ * filespec class
+ */
+class phpbb_mock_fileupload
+{
+ public $max_filesize = 100;
+ public $error_prefix = '';
+
+ public function valid_dimensions($filespec)
+ {
+ return true;
+ }
+
+ public function image_types()
@p

p Jul 7, 2012

Contributor

Add a comment explaining what this function is for/what it does.

@p p and 2 others commented on an outdated diff Jul 7, 2012

tests/uploads/filespec_test.php
@@ -0,0 +1,278 @@
+<?php
+/**
+ *
+ * @package testing
+ * @copyright (c) 2012 phpBB Group
+ * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+ *
+ */
+
+require_once __DIR__ . '/../../phpBB/includes/functions.php';
+require_once __DIR__ . '/../../phpBB/includes/utf/utf_tools.php';
+require_once __DIR__ . '/../../phpBB/includes/functions_upload.php';
+require_once __DIR__ . '/../mock/fileupload.php';
+require_once __DIR__ . '/../mock/request.php';
@p

p Jul 7, 2012

Contributor

Autoloading?

@Fyorl

Fyorl Jul 8, 2012

Contributor

They are not autoloaded, removing these lines results in a fatal error.

@bantu

bantu Jul 8, 2012

Member

Indeed. I also thought mocks could be autoloaded. But that is not the case.

@p p commented on an outdated diff Jul 7, 2012

tests/uploads/filespec_test.php
+
+ protected function setUp()
+ {
+ // Global $config required by unique_id
+ // Global $user required by filespec::additional_checks and
+ // filespec::move_file
+ global $config, $user;
+
+ if (!is_array($config))
+ {
+ $config = array();
+ }
+
+ $config['rand_seed'] = '';
+ $config['rand_seed_last_update'] = time() + 600;
+ $config['mime_triggers'] = 'body|head|html|img|plaintext|a href|pre|script|table|title';
@p

p Jul 7, 2012

Contributor

Add a comment explaining the choice of these tags. Is this everything that phpbb supports/uses? Minimum needed for the test to work? Specific values need for testing something? Etc.

@p p and 2 others commented on an outdated diff Jul 7, 2012

tests/uploads/filespec_test.php
+
+ $files = array(
+ 'gif_copy',
+ 'jpg_copy',
+ 'png_copy',
+ 'txt_copy',
+ 'txt_copy_2',
+ 'tif_copy',
+ 'gif_moved',
+ 'jpg_moved',
+ 'png_moved',
+ 'txt_as_img',
+ 'txt_moved',
+ );
+
+ foreach ($files as $file)
@p

p Jul 7, 2012

Contributor

Write all files in a subdirectory and nuke everything in that subdirectory.

@Fyorl

Fyorl Jul 8, 2012

Contributor

bantu, I think, asked for an explicit list of files to be removed. He may have meant it for some other issue that no longer exists now. A subdirectory sounds reasonable though.

@p

p Jul 8, 2012

Contributor

If this list changes then the test will stop deleting files and old files will stick around forever.

At the same time you don't want to accidentally delete files not created by this test, which is why I said to put everything into a subdirectory that nothing else should use.

@bantu

bantu Jul 8, 2012

Member

A subdirectory seems okay to me.

@p p and 1 other commented on an outdated diff Jul 7, 2012

tests/uploads/filespec_test.php
+ {
+ // Initialise a blank filespec object for use with trivial methods
+ $upload_ary = array(
+ 'name' => '',
+ 'type' => '',
+ 'size' => '',
+ 'tmp_name' => '',
+ 'error' => '',
+ );
+
+ return new filespec(array_merge($upload_ary, $override), null);
+ }
+
+ protected function tearDown()
+ {
+ global $user;
@p

p Jul 7, 2012

Contributor

Never used in this function.

@Fyorl

Fyorl Jul 8, 2012

Contributor

In a previous comment on this PR you asked for all globals to be cleared after use. So $user is being cleared here.

@p

p Jul 8, 2012

Contributor

Right, I overlooked the clearing.

@p

p Jul 8, 2012

Contributor

But you should add a comment explaining this.

@p p commented on an outdated diff Jul 7, 2012

tests/uploads/filespec_test.php
+ 'jpg_moved',
+ 'png_moved',
+ 'txt_as_img',
+ 'txt_moved',
+ );
+
+ foreach ($files as $file)
+ {
+ @unlink($this->path . $file);
+ }
+ }
+
+ public function additional_checks_variables()
+ {
+ return array(
+ array('gif', true),
@p

p Jul 7, 2012

Contributor

Add a comment explaining what true/false mean.

@p p and 2 others commented on an outdated diff Jul 7, 2012

tests/uploads/fixture/txt
@@ -0,0 +1 @@
+<HTML>mime trigger</HTML>
@p

p Jul 7, 2012

Contributor

html tags should be lowercase.

@Fyorl

Fyorl Jul 8, 2012

Contributor

These are uppercase to test whether the checks for disallowed content are properly case-insensitive.

@p

p Jul 8, 2012

Contributor

Is there an assertion that will fail should someone lowercase these tags?

@naderman

naderman Jul 8, 2012

Owner

Can you explain that? Are you now suggesting we test our tests to check that they test correctly? That seems a bit over the top.

@p

p Jul 9, 2012

Contributor

Either assertion or a comment. Currently there is no indication in the tree that those particular tags should remain uppercased.

@naderman

naderman Jul 9, 2012

Owner

Yeah I suppose a comment would make sense.

This pull request fails (merged 4cfacf1b into b6a364b).

This pull request fails (merged b323fd24 into b6a364b).

This pull request passes (merged 8f997f1a into b6a364b).

Member

bantu commented Jul 8, 2012

I think the uploads folder should maybe be called upload.

Member

bantu commented Jul 8, 2012

Please explain why the common.php file is required here.

Contributor

Fyorl commented Jul 8, 2012

I thought it was required for phpbb_chmod to work but I think it manages to locate the correct common.php in the phpBB directory so I have removed the common.php here.

This pull request fails (merged 73f2a449 into b6a364b).

@bantu bantu commented on an outdated diff Jul 8, 2012

tests/upload/filespec_test.php
@@ -0,0 +1,272 @@
+<?php
+/**
+ *
+ * @package testing
+ * @copyright (c) 2012 phpBB Group
+ * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+ *
+ */
+
+require_once __DIR__ . '/../../phpBB/includes/functions.php';
+require_once __DIR__ . '/../../phpBB/includes/utf/utf_tools.php';
+require_once __DIR__ . '/../../phpBB/includes/functions_upload.php';
+require_once __DIR__ . '/../mock/fileupload.php';
@bantu

bantu Jul 8, 2012

Member

Autoloading for mocks is now available in develop. http://tracker.phpbb.com/browse/PHPBB3-10973

Fyorl added some commits Jun 20, 2012

[ticket/10941] Minor adjustments as per PR comments
Switched from dirname(__FILE__) to just __DIR__.
Removed static definition from data provider methods.

PHPBB3-10941

This pull request fails (merged de0376f6 into 9f0259c).

@bantu bantu commented on an outdated diff Jul 8, 2012

tests/functional/fileupload_test_form.php
+
+ public function test_invalid_extension()
+ {
+ $crawler = $this->request('GET', 'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid);
+ $form = $crawler->selectButton('add_file')->form();
+ $form['fileupload']->upload($this->path . 'illegal-extension.bif');
+ $crawler = $this->client->submit($form);
+ $this->assertEquals('The extension bif is not allowed.', $crawler->filter('p.error')->text());
+ }
+
+ public function test_too_large()
+ {
+ $this->markTestIncomplete('Functional tests use an admin account which ignores maximum upload size.');
+ $crawler = $this->request('GET', 'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid);
+ $form = $crawler->selectButton('add_file')->form();
+ $form['fileupload']->upload($path . 'too-large.png');
@bantu

bantu Jul 8, 2012

Member

Should be $this->path instead of $path

This pull request fails (merged 47d45d19 into 9f0259c).

@bantu bantu commented on an outdated diff Jul 9, 2012

tests/functional/fileupload_test_form.php
+ {
+ $crawler = $this->request('GET', 'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid);
+ $form = $crawler->selectButton('add_file')->form();
+ $form['fileupload']->upload($this->path . 'illegal-extension.bif');
+ $crawler = $this->client->submit($form);
+ $this->assertEquals('The extension bif is not allowed.', $crawler->filter('p.error')->text());
+ }
+
+ public function test_too_large()
+ {
+ $this->markTestIncomplete('Functional tests use an admin account which ignores maximum upload size.');
+ $crawler = $this->request('GET', 'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid);
+ $form = $crawler->selectButton('add_file')->form();
+ $form['fileupload']->upload($path . 'too-large.png');
+ $crawler = $this->client->submit($form);
+ $this->assertEquals(1, $crawler->filter('div#message')->count());
@bantu

bantu Jul 9, 2012

Member

You should also check for the actual message here, although this test is marked incomplete.

This pull request fails (merged f60f0e45 into 9f0259c).

This pull request fails (merged f57d1733 into 9f0259c).

This pull request fails (merged 90c6a811 into 9f0259c).

This pull request passes (merged c8576327 into 9f0259c).

This pull request fails (merged 81259e75 into 9f0259c).

This pull request fails (merged b3fb5e5b into 9f0259c).

This pull request passes (merged 8e1f35a0 into 9f0259c).

Fyorl added some commits Jun 22, 2012

[ticket/10941] Added functional tests for the fileupload class
NOTE: test_form_upload() is broken. Uploading files via Symfony fails to
retain $_FILES['fileupload']['type'] even if it set explicitely. This appears
to be a bug in Symfony. Since the current version of filespec::is_image()
relies on the mimetype, these tests will __fail__. filespec::is_image() has
been fixed in #833 however.

PHPBB3-10941
[ticket/10941] Minor adjustments as per PR comments
Added some comments clarifying globals and lowercased fixture filenames

PHPBB3-10941
[ticket/10941] tearDown() now uses explicit file list
Instances of $it also renamed for clarity.

PHPBB3-10941
[ticket/10941] Now actually checks for the value of errors.
Uses phpbb_mock_lang to return the key used when setting errors to
allow that key to be checked for during tests rather than just checking
if any error was set.

PHPBB3-10941
[ticket/10941] Minor typo fixes
Removed superfluous $user = null; that was left over from refactoring.

PHPBB3-10941
[ticket/10941] Refactored init_filespec to return new object.
Removed $filespec as a property of the filespec test and instead
just instantiate new objects.

PHPBB3-10941
[ticket/10941] Removed the incomplete mark as is_image is fixed
Had to remove one of the tests due to a small limitation with the
functional testing framework. May mark the test as incomplete again
pending further comments.

PHPBB3-10941
[ticket/10941] Fixed a failing test
Since the is_image change, the filespec class no longer trusts the
browser's reported mimetype and instead checks the file itself. Thus
pretending a text file had the mimetype of an image will no longer
fail, the file will simply not be treated as an image.

PHPBB3-10941
[ticket/10941] Added subdirectory for file operations
Also removed common.php as it was unnecessary.

PHPBB3-10941
[ticket/10941] Removed manual includes of mock classes
Also marked a test as incomplete even though this appears to be
ignored when actually running the tests.

PHPBB3-10941
[ticket/10941] Renamed classes and filenames so that tests run
Also fixed some minor issues that weren't flagged before because
the tests were being ignored.

PHPBB3-10941

@bantu bantu merged commit 65d7aae into phpbb:develop Jul 9, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment