Skip to content

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Nov 4, 2024

When I'm debugging an issue with PHPStan, and want to go in deep into the NodeScopeResolver, I'm always surprised that the first breakpoints are triggered by this test. It's because work is done in the data provider that calls the NodeScopeResolver.

I think no work should be done in the PHPUnit data providers, because these are always executed. Even when you specify a filter to only run a specific test.

The total time of this test is still the same.

When I'm debugging an issue with PHPStan, and want to go in deep into the
NodeScopeResolver, I'm always surprised that the first breakpoints are triggered
by this test. It's because work is done in the data provider that calls the NodeScopeResolver.

I think no work should be done in the PHPUnit data providers, because these are always executed.
Even when you specify a filter to only run a specific test.
@ruudk ruudk changed the title Do not trigger NodeScopeResolver in data provider Do not trigger NodeScopeResolver in PHPUnit data provider Nov 4, 2024
@ruudk ruudk changed the base branch from 2.0.x to 1.12.x November 4, 2024 15:08
@ondrejmirtes
Copy link
Member

Sorry to be pain about this, but legacy is legacy, I prefer not to invest too much into that code. I'd much prefer if this was rewritten for NodeScopeResolverTest. Which means to move the asserted file into tests/PHPStan/Analyser/nsrt, and then transform the asserts performed in LegacyNodeScopeResolverTest into assertType and assertVariableCertainty calls.

All of this should be done with a script in an automated way, so I can review both the script and the results :)

Some test cases will be definitely harder to rewrite, but most of them should be easy.

@ruudk
Copy link
Contributor Author

ruudk commented Nov 4, 2024

I understand.

But writing a script to automatically convert these legacy tests to the new way seems like a lot of work which I can't allocate time for at the moment. Sorry.

Should this be closed?

I feel it's too bad, because it will impact everyone using the debugger in phpstan-src.

@ondrejmirtes
Copy link
Member

I'm sorry, I worry this wouldn't work too well. Every time a new static property is introduced, something breaks somewhere. For example a file can be tested multiple times with different configuration. I also don't want to invest in modernizing LegacyNodeScopeResolver. I want to get rid of it.

When I'm debugging something, I always run PHPStan analysis on that file by running bin/phpstan. Not through PHPUnit.

@ruudk ruudk deleted the tweak-legacy-node-scope-resolver-test branch November 5, 2024 12:08
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