Last modified: 2010-02-02 20:59:53 UTC
Created attachment 6429 [details] patch to fix 5 bugs in .../phase3/tests/ directory and to add a --runall option to run-tests.php The attached patch fixes several bugs in and adds one feature to /phase3/tests/. (BEFORE COMMITTING THIS PATCH, PLEASE READ THE ARCHITECTURAL DISCUSSION IN THE 3 PARAGRAPHS AT END OF THIS TEXT) The patch is merged, diffed and tested against r54465. The following files are affected: * run-test.php: fixed bug related to the pathname to the PHPUnit directory. Added a --help command line option and a --runall option. The latter allows run-test to execute all PHPUnit tests in the /tests/ directory. In order to utilize this option and satisfy other constraints (see below for discussion) I renamed SearchEngineTest.php to SearchEngineTest.inc. This makes sense since SearchEngineTest.inc contains only an abstract class, not directly executable code. This patch adds SearchEngineTest.inc and deletes SearchEngineTest.php from the distribution. * ArticleTest.php: fixed bug related to global variables that may or may not exist. Tests for global variable existence before attempting to save it for later restoration. * LocalFileTest.php: fixed bug related to a change in the directory structure used to store thumbnails in file based repositories. * MediaWiki_TestCase.php: fixed bug related to a change in the name of the global variables $wgDBadminuser (-> $wgDBuser) and $wgDBadminpassword (->$wgDBpassword). Also fixed bug that wrote over $wgDBprefix, causing creation of test tables to fail. Tested code for both MySQL version >= 4.1 and < 4.1 (by creating temporary debug version of if statement - tests occured using MySQL version 5.0.41). * README: Updated so it mentions requirement to set $PhpUnitDirectory in run-test.php and explains use of new --runall option. * SearchEngineTest.php: renamed to SearchEngineTest.inc so algorithm in run-tests.php that automatically finds all PHPUnit tests works (see below). * SearchMySQL4Test: renamed 'SearchEngineTest.php' to 'SearchEngineTest.inc' in require_once() call. There is an architectural issue related to this patch that needs attention by senior developers. I decided to add the --runall option to run-test.php so we can run the PHPUnit tests without relying on the make utility. However, in order to retain the ability to use the current Makefile, I used an algorithm to find the PHPUnit tests in the /tests/ directory that isn't very pretty. Specifically, the algorithm assumes all files of the form <prefix>Test.php are PHPUnit tests (e.g., ArticleTest.php). This follows the convention used by PHPUnit tests that the unit test for class <class> is named <class>Test. This convention was used by the developer who initially created the PHPUnit tests for file names as well. A better architectural approach would be to create a subdirectory of /tests/, say PHPUnitTests, that contains all PHPUnit tests. This, however, requires a structural change to the /tests/ directory that is sufficiently major that it requires review. If this approach is considered superior, it would be easy to modify run-tests.php to run all tests in the PHPUnitTests sub-directory. If senior developers give me feedback in bug ticket comments that this strategy is better, I can provide an updated patch fairly quickly. Another architectural approach is to create a file in /tests/ that lists the PHPUnit tests to run. This has the disadvantage that it requires updating whenever a new test is added. My view is this is a less desirable option.
Just some early feedback, haven't looked into the run all stuff yet. (In reply to comment #0) > * ArticleTest.php: fixed bug related to global variables that may or may not > exist. Tests for global variable existence before attempting to save it for > later restoration. > > * LocalFileTest.php: fixed bug related to a change in the directory structure > used to store thumbnails in file based repositories. > These two fixes look fine. > * MediaWiki_TestCase.php: fixed bug related to a change in the name of the > global variables $wgDBadminuser (-> $wgDBuser) and $wgDBadminpassword > (->$wgDBpassword). Also fixed bug that wrote over $wgDBprefix, causing creation > of test tables to fail. Tested code for both MySQL version >= 4.1 and < 4.1 (by > creating temporary debug version of if statement - tests occured using MySQL > version 5.0.41). > This is incorrect. $wgDBadminuser/$wgDBadminpassword are used for attaining "administrative" access to the DB. In practice, most scripts don't need this. Since we're creating temp tables though, it's probably worth using $wgDBadminuser here, we don't know if $wgDBuser has access to do that. > * README: Updated so it mentions requirement to set $PhpUnitDirectory in > run-test.php and explains use of new --runall option. > > * SearchEngineTest.php: renamed to SearchEngineTest.inc so algorithm in > run-tests.php that automatically finds all PHPUnit tests works (see below). > > * SearchMySQL4Test: renamed 'SearchEngineTest.php' to 'SearchEngineTest.inc' in > require_once() call. > This all looks fine too. > There is an architectural issue related to this patch that needs attention by > senior developers. I decided to add the --runall option to run-test.php so we > can run the PHPUnit tests without relying on the make utility. However, in > order to retain the ability to use the current Makefile, I used an algorithm to > find the PHPUnit tests in the /tests/ directory that isn't very pretty. > Specifically, the algorithm assumes all files of the form <prefix>Test.php are > PHPUnit tests (e.g., ArticleTest.php). This follows the convention used by > PHPUnit tests that the unit test for class <class> is named <class>Test. This > convention was used by the developer who initially created the PHPUnit tests > for file names as well. > > A better architectural approach would be to create a subdirectory of /tests/, > say PHPUnitTests, that contains all PHPUnit tests. This, however, requires a > structural change to the /tests/ directory that is sufficiently major that it > requires review. If this approach is considered superior, it would be easy to > modify run-tests.php to run all tests in the PHPUnitTests sub-directory. If > senior developers give me feedback in bug ticket comments that this strategy is > better, I can provide an updated patch fairly quickly. > Moving the tests to a subdirectory would be fine, and pretty trivial as long as all referencing paths are updated too. > Another architectural approach is to create a file in /tests/ that lists the > PHPUnit tests to run. This has the disadvantage that it requires updating > whenever a new test is added. My view is this is a less desirable option. > Ew no. That's the exact thing we're trying to avoid doing...we've got way too many "lists of" type configs out there, and it's getting ridiculous. Also, something I noticed: $PhpUnitDirectory is nasty and should be removed. A) PEAR should already be in the include path B) We don't like telling people to edit any files other than LocalSettings, as changes can get lost on upgrade.
(In reply to comment #1) > > * MediaWiki_TestCase.php: fixed bug related to a change in the name of the > > global variables $wgDBadminuser (-> $wgDBuser) and $wgDBadminpassword > > (->$wgDBpassword). Also fixed bug that wrote over $wgDBprefix, causing creation > > of test tables to fail. Tested code for both MySQL version >= 4.1 and < 4.1 (by > > creating temporary debug version of if statement - tests occured using MySQL > > version 5.0.41). > > > > This is incorrect. $wgDBadminuser/$wgDBadminpassword are used for attaining > "administrative" access to the DB. In practice, most scripts don't need this. > Since we're creating temp tables though, it's probably worth using > $wgDBadminuser here, we don't know if $wgDBuser has access to do that. > OK. I will change them back. However, in order to test them I have to create $wgDBadminuser and $wgDBadminpassword for my local test wiki. Any pointers on how to do this? From your comments on wikitech I just set them in LocalSettings.php - correct? > Moving the tests to a subdirectory would be fine, and pretty trivial as long as > all referencing paths are updated too. > I will start working on this. > > Also, something I noticed: $PhpUnitDirectory is nasty and should be removed. A) > PEAR should already be in the include path B) We don't like telling people to > edit any files other than LocalSettings, as changes can get lost on upgrade. > OK. However, on my system I had to define this variable in order to find the files in PHPUnit (e.g., TextUI/Command.php). My system may be a bit unusual since the OS comes with php4 installed. I use MAMP, which comes with php5, but in order to override php4, I had to do a bunch of softlinking and aliasing. I don't know if this is inadequate in order to get PEAR in the include path or something else is wrong, but until I can figure this out, I can't test the elimination of $PhpUnitDirectory. Any suggestions?
(In reply to comment #2) > (In reply to comment #1) > > OK. However, on my system I had to define this variable in order to find the > files in PHPUnit (e.g., TextUI/Command.php). My system may be a bit unusual > since the OS comes with php4 installed. I use MAMP, which comes with php5, but > in order to override php4, I had to do a bunch of softlinking and aliasing. I > don't > know if this is inadequate in order to get PEAR in the include path or > something > else is wrong, but until I can figure this out, I can't test the elimination of > $PhpUnitDirectory. Any suggestions? > Figured it out. I had to set include_path in php.ini to include the path to PHPUnit.
Created attachment 6442 [details] fixes 3 problems specified in reply #1.
Created attachment 6443 [details] updated patch to correct problem in run-test.php <list> case No sooner had I provided the updated patch than I realized I forgot to test the run-test.php <list> case. As normal, there was a problem. The attached patch fixes it and supersedes tests-dir.patch
I inappropriately got in a hurry last night. I should have waited until I thoroughly reviewed my changes. DO NOT COMMIT my patch until I do so.
Created attachment 6451 [details] modifed patch to fix bugs and add --runall option to run-test.php Patch tests-dir3.patch fixes the bugs described in this bug ticket. It modifies the changes contained in testsPatch.patch and all others (superceded by this patch) according to the review in comment #1 of this ticket. The patch is merged, diffed and tested against r54724. The following changes are made: * all PHPUnit tests (specifically, ArticleTest.php, DatabaseTest.php, GlobalTest.php, ImageFunctionsTest.php, LocalFileTest.php and SearchMySQL4Test.php) are moved to the sub-directory /PHPUnitTests/ of the directory /tests/. The patch makes corresponding changes to run-test.php and some test files so they operate correctly out of this directory. * run-test.php: rolled back change in testsPatch.patch that required setting the directory to the PHPUnit directory. Tests in the tests directory assume this directory is accessible, e.g., it is one of the include_path directories set in php.ini. Added a --help command line option and a --runall option. The latter allows run-test to execute all PHPUnit tests in the .../tests/PHPUnitTests/ directory. In order to utilize this option and satisfy other constraints (see below for discussion) the patch renames SearchEngineTest.php to SearchEngineTest.inc, retaining it in the /tests/ directory. This makes sense since SearchEngineTest.inc contains only an abstract class, not directly executable code. The patch adds SearchEngineTest.inc and deletes SearchEngineTest.php from the distribution. Finally, the patch modifies the help text to reflect the new directory organization of /tests/. * ArticleTest.php: fixed bug related to backing up global variables that may not exist. Tests for global variable existence before attempting to save it for later restoration. * LocalFileTest.php: fixed bug related to a change in the directory structure used to store thumbnails in file based repositories. * MediaWiki_TestCase.php: fixed bug that wrote over $wgDBprefix, causing creation of test tables to fail. Tested code for both MySQL version >= 4.1 and < 4.1 (by creating temporary debug version of if statement - tests occured using MySQL version 5.0.41). Rolled back the change in testsPatch.patch that changed the name of the global variables $wgDBadminuser (-> $wgDBuser) and $wgDBadminpassword (->$wgDBpassword). The patch assumes these are set in LocalSettings.php. * README: Updated so it describes the new PHPUnitTests directory and explains use of the new --runall option. * SearchEngineTest.php: renamed to SearchEngineTest.inc and retained in the /tests/ directory so algorithm in run-tests.php that automatically finds all PHPUnit tests works. * SearchMySQL4Test: renamed 'SearchEngineTest.php' to 'SearchEngineTest.inc' in require_once() call. * Makefile: added LocalFileTest.php to the all/test target. Commented out install and clean targets (which attempted to install and remove PHPUnit from the installation) and their explanation in the help text.
I'm investigating how to unify the tests in /tests/ and /t/. During this, I noticed there is a fossil in run-tests.php (as modified by tests-dir3.patch). The ini_set() on line 3 is no longer needed. I can either provide a new patch or the commiter can delete it. Let me know.
After some discussion with Ævar, I've decided that the tests in /t are actually nicer than the ones in /tests, and they don't require the user to have PHPUnit. I think most of /tests is already covered in /t already, things that aren't should be moved over. As far as I'm concerned, /tests should go away and that makes this a WONTFIX.
mah was the only one who felt like fixing the tests. He chose PHPUnit ones. Changing WONTFIX --> FIXED :D
