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

prepare to upgrade to PHPUnit 10. #5093

Merged
merged 10 commits into from Dec 11, 2023
Merged

Conversation

stopfstedt
Copy link
Member

@stopfstedt stopfstedt commented Dec 1, 2023

PHPUnit v10 is still a no-go, b/c the PHPUnit/Symfony bridge isn't ready for it yet.

we can still frontload the upgrade by addressing all deprecations that would result in breakage:

  • data provider need to be public and static in v10, this makes it so.
  • all test methods relying on data providers where the provider yields no data need to be marked as skipped, in order to prevent test failures in v10.

@stopfstedt stopfstedt force-pushed the phpunit-10 branch 3 times, most recently from 67f1dd8 to d909d2e Compare December 4, 2023 18:21
@stopfstedt stopfstedt changed the title upgrade to PHPUnit 10. prepare to upgrade to PHPUnit 10. Dec 4, 2023
@stopfstedt stopfstedt force-pushed the phpunit-10 branch 3 times, most recently from b9c68b5 to 3eba810 Compare December 4, 2023 19:35
@stopfstedt stopfstedt marked this pull request as ready for review December 4, 2023 20:53
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

Big step! Couple of questions.

public function graphQLFiltersToTest(): array

/**
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

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

I could do without these since we don't actually use any PHP doc parsing.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair. i yanked all of these from the endpoint tests.

tests/QEndpointTrait.php Show resolved Hide resolved
* Class JsonControllerTest
*/
trait JsonControllerTest
trait JsonControllerTestable
Copy link
Member

Choose a reason for hiding this comment

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

Maybe being weird, but I don't love this name... maybe TestableJsonController?

Copy link
Member Author

Choose a reason for hiding this comment

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

you go it.

@@ -185,16 +185,16 @@ public function testCreateJwtFromServiceTokenUser()
$this->assertEquals($this->obj->getExpiresAtFromToken($jwt)->getTimestamp(), $expiresAt->getTimestamp());
}

public function getWriteableSchoolIdsFromTokenProvider(): array
public static function getWriteableSchoolIdsFromTokenProvider(): array
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch why this is static now, is it used in the providers?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that method is a data provider. they must to be static and public in PHPUnit v10.

@@ -17,22 +17,7 @@ public function setUp(): void
$this->voter = new Voter();
}

public function supportsTypeProvider(): array
Copy link
Member

Choose a reason for hiding this comment

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

Am I understanding correctly that this went away because it's only used in GreenLightViewDTOVoter and ReadonOnlyDTOVoter so isn't inherited elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

this had to be re-tooled, unfortunately. since the previous method was invoking an abstract method (another data provider), which isn't allowed with abstract static methods.
so i had to push this down into the concrete test classes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah that tracks. 🆒

dependabot bot and others added 10 commits December 11, 2023 10:50
Bumps [phpunit/phpunit](https://github.com/sebastianbergmann/phpunit) from 9.6.15 to 10.5.1.
- [Changelog](https://github.com/sebastianbergmann/phpunit/blob/10.5.1/ChangeLog-10.5.md)
- [Commits](sebastianbergmann/phpunit@9.6.15...10.5.1)

---
updated-dependencies:
- dependency-name: phpunit/phpunit
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
fill these up with actual test vectors.
- replaces <coverage> with <source>
- fail on deprecation instead of converting them to exceptions
- rm Symfony Bridge listener without replacement since the listener
subsystem has been removed without replacement
- rm Symfony bridge related env vars
i missed this one during previous passes over this.
the old name was confusing PHPUnit, resulting in a test runner warning.
this has become necessary to prevent these tests from failing.
the symfony bridge isn't ready for v10, let's keep all the code that
addresses deprecations/breaking changes with v10 and revert back to v9.
we're not generating docs from test coverage, so this is kinda
pointless.
removed as requested.
@dartajax dartajax merged commit bc43145 into ilios:master Dec 11, 2023
35 checks passed
@stopfstedt stopfstedt deleted the phpunit-10 branch December 11, 2023 21:40
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.

None yet

3 participants