Unit test: fix tests failing #108
Conversation
|
Hm. While this passes in php7, it seems to fail in php 5 (see the CI tests which I finally got to work on contributor PRs). |
|
@sirbrillig Hmm... I'll have little play with that. The failure you are seeing on PHP 5 are what I was seeing on PHP 7 leading to this PR. |
|
@sirbrillig This is weird, I've just tried to reproduce the errors you are seeing on circleCI locally and cannot with this branch.
|
|
Also working perfectly fine with PHP 7 and PHPUnit 7.x locally:
|
jrfnl
force-pushed
the
feature/fix-failing-tests
branch
2 times, most recently
from
5e3ebb3
to
c9d6df1
Compare
|
🤔 This is really weird. I suppose there must be something different about the CI environment. I'm trying to reproduce this myself but I'm having trouble getting php 5 installed again. I used to have a way to do this but I've forgotten and/or homebrew has changed how it manages php. Do you use homebrew? If so, how do you switch? |
|
@sirbrillig No, I'm on Windows, so no homebrew here. I've got a ridiculous amount of PHP versions installed so I can switch between them with ease and used the same PHP + PHPUnit version as CircleCI does when I tested locally on PHP 5. |
|
@sirbrillig I'm going to run some tests to see which particular bit of the change is causing the CircleCI failure, so I'll be force pushing to this branch over and over till I find it. I'll report back when I know more. Please ignore all the push notifications you'll be getting ;-) |
jrfnl
force-pushed
the
feature/fix-failing-tests
branch
3 times, most recently
from
5f0d172
to
4475ce5
Compare
|
Ha! I've found why the tests were failing when I ran them locally. It had to do with the mixed forward/backward slashes in the Now off to see if I can find a simple solution which works cross-platfom. |
jrfnl
force-pushed
the
feature/fix-failing-tests
branch
from
4475ce5
to
ad884fb
Compare
|
@sirbrillig Ok, so I can now change this to the most minimal change to get it working cross-platform or I can still try to simplify the tests a little more and keep the PR a little more comprehensive than just the minimum. Got a preference ? |
|
Nice sleuthing. Let's start with the minimum to make things work and then make the simplification its own PR. I really like that simplification, BTW. That said, is this ready? |
The tests would fail on Windows due to the mix of forward and backward slashes in the sniff file name. This simple change should allow the tests to work cross-platform.
jrfnl
force-pushed
the
feature/fix-failing-tests
branch
from
ad884fb
to
ee263e8
Compare
|
@sirbrillig It is now ;-) I've force pushed just the commit with the simplest fix to this branch now (ee263e8). If CircleCI passes, it's ready to be merged. |
I'm not completely sure why, but when I tried to run the unit tests locally, most were failing.
This minor tweak in how PHPCS is instantiated fixes it. It also removes the need for the
getSniffFiles()method as there is only one sniff in this standard anyway.