Re: PHPT SPL

Hi, On Thu, 2010-03-11 at 17:27 +0000, chris b wrote: > Hi, > > I'd like to contribute some PHPT tests for SPL. They've been tested on > the most recent PHP5.2/PHP5.3/PHP6 snapshot builds. They can be found > at: > > http://www.2shared.com/file/12032337/506959a0/spl.html Just a quick glance over the tests after finding the small link on the page to get the file, without looking at all tests or even running them: cachingIterator_basic.phpt: | $cache1 = new CachingIterator(new ArrayIterator($array1)); | | //check hasNext | var_dump($cache1->hasNext()); Without rewind the state of an iterator (especially an outer iterator) is "undefined." That should be made clearer in the docs, I'd drop the call from the test or do a rewind first. cachingIterator_constants.phpt: I would treat the actual values as implementation detail, too. Not sure what we're doing in other tests. directoryIterator_basic_001.pht: Please use __DIR__ as basis for other temporary files and name them something related to the test - I had a few situations in the past where some cleanup didn't work and it was hard to find the matching test. where. Might affect other tests, too. directoryIterator_error.phpt: A directory called '8' might exist, better use some most likely invalid name. The exact message ("failed to open dir: No such file or directory") is operating system and locale dependent and should not be tested. The test should also test for the right Exception type. norewinditerator.phpt: Might make sense to check the valid() return value after the manual rewind. Maybe a good test might be to extend NoRewindIterator overriding rewind() to verify it being called. The currentone would still be needed as overridden iterators can use different implementations depending whether they were overwritten or not. class testNorewindIterator extends NoRewindIterator { public function rewind() { echo"Rewinding!\n"; } } As said these were just some random picks, overall these are small issues and the tests at large look like good work! johannes