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

CI: Only enable Whoops when it is tested #5554

Merged
merged 1 commit into from Sep 2, 2023

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Sep 1, 2023

This first step already helps us to make CI succeed on PHP 8.0. I'll still work on the other quick fix as we discussed, but that may take a little longer.

This PR …

Refactoring

  • Whoops is now generally disabled during PHPUnit test runs to reduce memory usage during tests

Breaking changes

None

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

Reduces the PHPUnit memory usage because Whoops introduces memory leaks
@lukasbestle lukasbestle added type: refactoring ♻️ Is about bad code; cleans up code type: tests 🧪 Is about missing tests; increases test coverage or improves tests labels Sep 1, 2023
@lukasbestle lukasbestle added this to the 4.0.0-beta.1 milestone Sep 1, 2023
@lukasbestle lukasbestle self-assigned this Sep 1, 2023
Copy link
Member

@distantnative distantnative left a comment

Choose a reason for hiding this comment

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

Works for me locally and I implementation looks quite clean.

Didn't quite understand the Assert/Deprecated PHPUnit stuff, but guess that's intended/fine.

@lukasbestle
Copy link
Member Author

The Error\Deprecated and Error\Warning classes are exceptions thrown by PHPUnit when a PHP (deprecation) warning was triggered. So by catching those, we can test for the warnings.

I first used $this->expectException(Deprecated::class), however that lead to a PHPUnit deprecation warning by itself because PHPUnit 10 will no longer convert warnings to exceptions. That's why the try/catch is necessary for now. We'll need yet another solution once we switch to PHPUnit 10: sebastianbergmann/phpunit#5062

Assert::fail() throws an exception that makes the test fail. That's needed to mark the test as failed if the catch didn't run as expected.

@distantnative distantnative merged commit 6d6b4f7 into v4/develop Sep 2, 2023
12 checks passed
@distantnative distantnative deleted the feature/whoops-ci branch September 2, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code type: tests 🧪 Is about missing tests; increases test coverage or improves tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants