Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests. #24

Closed
wants to merge 7 commits into from
Closed

Fix tests. #24

wants to merge 7 commits into from

Conversation

SpacePossum
Copy link

@SpacePossum SpacePossum commented Feb 21, 2017

somehow the PHP check of the fixer itself must be bypassed (https://travis-ci.org/keradus/PHP-CS-Fixer/jobs/203770470#L292) or it should not run as phar on the 7.2/nightly test suit.

@SpacePossum
Copy link
Author

@keradus
https://travis-ci.org/keradus/PHP-CS-Fixer/jobs/205701196#L915
interesting failure, I wonder if we should call next() first... maybe this is a odd diff. between PHP/Zend and HHVM?

.travis.yml Outdated
@@ -82,8 +82,7 @@ script:
- if [ $TASK_TESTS == 1 ] && [ $TASK_TESTS_COVERAGE == 0 ]; then vendor/bin/phpunit --verbose; fi
- if [ $TASK_TESTS == 1 ] && [ $TASK_TESTS_COVERAGE == 1 ]; then phpdbg -qrr vendor/bin/phpunit --verbose --coverage-clover build/logs/clover.xml; fi

- if [ $TASK_SCA == 1 ]; then git checkout . -q; fi

- if [ $TASK_CS == 1 ]; then git checkout . -q; fi
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? if no SCA was performed, there is no files to checkout, isn't it ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, this change is not good

.travis.yml Outdated
@@ -21,7 +21,7 @@ matrix:
- php: 7.1
env: TASK_SCA=1
- php: nightly
env: TASK_SCA=1 COMPOSER_FLAGS="--ignore-platform-reqs"
env: COMPOSER_FLAGS="--ignore-platform-reqs" TASK_CS=0
Copy link
Owner

@keradus keradus Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I was thinking actually was introduction of new ENV flag:
PHP_CS_FIXER_IGNORE_ENV=1 and allow tool to run on 7.2 with that flag.
For that, we would be able to run the SCA/CS tasks

@@ -107,7 +107,7 @@ public function testThatFinderWorksWithDirSetOnConfig()
$config = new Config();

$iterator = $config->getFinder()->in(__DIR__.'/Fixtures/FinderDirectory')->getIterator();
$this->assertSame(1, count($iterator));
$this->assertSame(1, iterator_count($iterator));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, do you have an ref for that change ?
would assertCount finally works ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertCount will work I think, however we might run in the old HHVM bug

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my question is more like "is there a hhvm version that has fixed that bug already" :D

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, it still not working on hhvm :/

@keradus
Copy link
Owner

keradus commented Feb 27, 2017

why calling next would help? then, we would drop one value from collection, wound't we ?

@SpacePossum
Copy link
Author

SpacePossum commented Feb 27, 2017

Indeed, calling next is wrong.

This fails for assertCount as well. However not using assertCount but iterator_count fails as well (so it is not sebastianbergmann/phpunit#2149). I start to believe that something is off between HHVM and PHP/Zend

@SpacePossum
Copy link
Author

My guess is https://3v4l.org/ZjdMV i.e. something like HHVM and RecursiveRegexIterator for ref.: facebook/hhvm#4966

@keradus keradus force-pushed the 2.1_travis branch 2 times, most recently from a24c8a4 to 476405d Compare February 27, 2017 19:11
@SpacePossum SpacePossum closed this Mar 7, 2017
@SpacePossum SpacePossum deleted the 2.1_travis_fix branch March 7, 2017 08:03
@@ -133,7 +133,7 @@ public function testThatCustomSymfonyFinderWorks()
$config = Config::create()->setFinder($finder);

$iterator = $config->getFinder()->getIterator();
$this->assertSame(1, count($iterator));
$this->assertCount(1, $iterator);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SpacePossum , fix to make it working properly on hhvm is to run iterator_to_array($iterator) first :/
still, even hhvm 3.18 is not fixed that bug... what a shame for hhvm...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wooow, so great :/

@keradus
Copy link
Owner

keradus commented Mar 7, 2017

@SpacePossum , could you review it please: PHP-CS-Fixer#2552 ?
Travis is green for both 2.1 and master lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants