Conversation
src/bootstrap.php
Outdated
| require __DIR__ . '/iter.rewindable.php'; No newline at end of file | ||
| require __DIR__ . '/iter.rewindable.php'; | ||
| require __DIR__ . '/../test/MyIterator.php'; | ||
| require __DIR__ . '/../test/MyIteratorAgg.php'; |
There was a problem hiding this comment.
These files shouldn't be included here, as they are only needed for tests. Either we should have a separate bootstrap file in the test directory, or we can explicitly include the files in the test, or we can place the class definitions in the test file (like _CountableTestDummy).
There was a problem hiding this comment.
All right, I will move the related test class bootstrap.php file to the test folder.
There was a problem hiding this comment.
I use the autoload-dev in composer.json to autoload the required *Test.php files.
test/iterTest.php
Outdated
| } | ||
|
|
||
| public function testCountWithStringCount() { | ||
| $this->assertSame(3, count(new MyIterator())); |
There was a problem hiding this comment.
I don't really understand the purpose of this test. This is testing counting of an iterator, which is also tested by the $this->assertSame(5, count(toIter([1, 2, 3, 4, 5]))) line above. I'm also not clear on why this is called "testCountWithStringCount". How is this related to strings?
There was a problem hiding this comment.
Yes, it's my fault to create the duplicated tests.
I will remove this test method.
test/iterTest.php
Outdated
| } | ||
|
|
||
| public function testIsEmptyWithValidIteratorAgg() { | ||
| $this->assertFalse(isEmpty(new MyIteratorAgg())); |
There was a problem hiding this comment.
I guess we should test count() with an iterator aggregate as well, looks like that case isn't present yet.
| - 7.2 | ||
|
|
||
| install: | ||
| - composer install --prefer-dist |
There was a problem hiding this comment.
How about moving this to composer.json via
$ composer config preferred-install dist
?
There was a problem hiding this comment.
@localheinz, thank you for your reply.
It looks like adding this --prefer-dist config in composer.json instead of letting this be the option in composer install command.
I think revert this option in composer install command is the proper way if this option is required.
What do you think? Thanks.
3 participants
changed log
IteratorandIteratorAggregateinterface to test thecountandisEmptyfunction..travis.yml.The default dist is trusty.Remove
--prefer-distbecause it should let composer check the dependency.