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

Implemented support for #[Test], #[DataProvider], #[Before] attributes, cleaned up old PHPUnit/Prophecy support #149

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Ocramius
Copy link

@Ocramius Ocramius commented Feb 8, 2025

I initially wanted to introduce PHPUnit attribute support, but the amount of effort needed (mostly due to complex test tooling in this package) led me to removing everything that isn't strictly necessary anymore.

I've added conflict clauses for all old/unsupported versions of PHPUnit / Prophecy.

I've yet to understand how to read an attribute via PHP-Parser: debugging with separate-process execution isn't properly "fun" :D

By removing `composer/package-versions-deprecated`, and instead using `composer-runtime-api:^2`,
we both get rid of a dependency, and obtain better type information from our lookup.

This also coincidentally drops a bunch of inline `@psalm-suppress` statements.
…PHPUnit version

Since PHPUnit 8.0, `expectException()` has proper type annotations, and I even patched `createMock()` and similars
so that proper generic types are inferred.

These stubs are therefore no longer necessary.

At this point, these classes are mostly "OK" in PHPUnit, starting ~8.5.1:

* `PHPUnit\Framework\MockObject\Builder\InvocationMocker` (now declares missing types)
* `PHPUnit\Framework\MockObject\MockObject` (still uses `@method` at class level, but that works)
* `PHPUnit\Framework\MockObject\MockBuilder` (fully templated)
* `PHPUnit\Framework\TestCase` (except for `prophesize()`, which was later removed)
Adding a conflict to PHPUnit earlier than 8.5.1, to make
the phpunit version requirement more clear.

Starting with PHPUnit 8.0, assertions have decent assertion metadata declarations
on most methods.

This also removes assertion tests, simplifying the test suite further.
Starting from Prophecy 1.20.0, methods have proper generic types declared.

Adding a conflict to prevent earlier prophecy versions from being co-installed.

Refs:

* phpspec/prophecy@e4d39b1
* phpspec/prophecy@0bb033e
* phpspec/prophecy@4506935
Starting from `phpspec/prophecy-phpunit:2.3.0`, the out-of-the-box `ProphecyTrait` is well typed.

Ref: phpspec/prophecy-phpunit@ba6dc1b
@Ocramius
Copy link
Author

Ocramius commented Feb 8, 2025

Ah, I need to get the AST before a ClassMethod: that may be tricky...

@Ocramius Ocramius force-pushed the feature/support-phpunit-attributes branch from 97f36b3 to 915026b Compare February 8, 2025 12:25
@Ocramius
Copy link
Author

Ocramius commented Feb 8, 2025

EDIT: outdated / done.

@danog locally, everything seems green now: mostly spent time deleting code (good!).

I'm hoping to make a second PR with the support for #[Before], #[DataProvider] and #[Test] attributes, but I need some guidance:

  • when inspecting a ClassMethod node, how would I go about getting the FQCN of an attribute?
  • is Psalm performing some sort of FQCN normalization pass on its own? Should I use \Psalm\Type::getFQCLNFromString()?

@Ocramius Ocramius changed the title Simplified plugin by removing ancient PHPUnit/Prophecy support Implemented support for #[Test], #[DataProvider], #[Before] attributes, cleaned up old PHPUnit/Prophecy support Feb 8, 2025
…deception-psalm-module`

These packages are runtimes that invoke `psalm/plugin-phpunit` via `exec()`, without sharing codespace with
it, and therefore are safe to externalize elsewhere.

We can't afford keeping these dependencies co-located with the main `composer.json` / `composer.lock`, as
they conflict with non-dev dependencies that we want to explicitly test against.
@Ocramius
Copy link
Author

Ocramius commented Feb 8, 2025

Patch is done, but it doesn't run due to Codeception toolinng not working.

I raised psalm/codeception-psalm-module#54, but am still not happy with it.

What I will likely do instead: will add Behat, will write a small context that replicates current steps, and will drop the external dependency. While it is indeed a bit aggressive (and good work was done here), the external dependency is too much for what we're testing here, and these scenario step runners should not be maintained outside this codebase.

@Ocramius
Copy link
Author

Ocramius commented Feb 8, 2025

I will actually pursue the Behat approach: it is a single class to write, and will still keep it under tools/, to prevent any propagation to dependency ranges.

While `psalm/codeception-psalm-module` is a good idea in theory,
in practice, Gherkin scenario steps as a composer dependency are really (REALLY)
hard to distribute and maintain together with a test suite.

This move replaces the external dependency with a local one, with the
test runner mostly staying out of the way, and test logic all implemented
in the `Context` class we wrote ourselves.

The main aim is to improve atomicity of changes on the test suite itself.

Ref: psalm/codeception-psalm-module#54
@Ocramius
Copy link
Author

Ocramius commented Feb 9, 2025

This is green on my branch now, AFAIK.

Poking @danog and @weirdan for feedback: worth moving forward, but I did indeed step on a lot of other people's feet for the sake of getting things polished my way, sorry about that :-\

@weirdan
Copy link
Member

weirdan commented Feb 10, 2025

I think it would be more beneficial to explore the solution within the current codeception infrastructures because it would help all plugins that already use codeception-psalm-module. It could very well be the case that your approach is the best because it breaks a lot of dependency cycles (good). I just don't see right now how to apply it in the wider context of the entire plugin ecosystem.

@Ocramius
Copy link
Author

I'm OK with such a decision, but I'm not going to go back to codeception myself :D

@weirdan
Copy link
Member

weirdan commented Feb 10, 2025

Oh, it's not a decision, it's just my thoughts. I'm on hiatus, @danog makes decisions now.

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