Skip to content

Conversation

@carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Nov 13, 2017

As did in others Reactphp repos, I use the PHPUnit\Framework\TestCase notation instead of PHPUnit_Framework_TestCase while extending our TestCases. This helped us migrate to PHPUnit 6, that no longer support snake case class names.

I just need to bump PHPUnit version to ^4.8.35 and ^5.7, that support this namespace.

@carusogabriel
Copy link
Contributor Author

Ok, we are facing some issues with this update. I have some solutions, and I want your opinion:

  • Just update to PHPUnit 5, because PHPUnit has big breaking changes;
  • Drop PHPUnit 4 support, but require minimum PHP 5.6;

@clue
Copy link
Member

clue commented Nov 18, 2017

@Gabriel-Caruso Thanks for keeping up with this! I've updated reactphp/socket#133 to add a very simple BC layer to support all PHPUnit versions, perhaps you can look into implementing a similar logic here? 👍

@carusogabriel
Copy link
Contributor Author

@clue Actually, no, because there's no setExpectedMessage method for PHPUnit 4. What should we do?

@clue
Copy link
Member

clue commented Nov 19, 2017

@Gabriel-Caruso Not sure I follow, have you seen the setExpectedException() BC layer from the above PR (https://github.com/reactphp/socket/pull/133/files#diff-d4c8c6dc8769324fc27cfdac19f05cafR85)? This works out of the box on PHPUnit 4 and will redirect to new method calls on newer versions of PHPUnit.

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Nov 19, 2017

@clue Done, everything is working now 🎉

@clue clue changed the title Use PHPUnit\Framework\TestCase instead of PHPUnit_Framework_TestCase Forward compatibility with PHPUnit 6 Nov 19, 2017
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Thanks for keeping up with this, changes LGTM! :shipit: 👍

@clue clue added this to the v0.4.12 milestone Nov 19, 2017
@carusogabriel
Copy link
Contributor Author

Anything else that should be done here?

@WyriHaximus
Copy link
Member

Nope LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants