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

DX: PsrAutoloadingFixerTest - do not build mock in data provider #7491

Merged
merged 2 commits into from Dec 2, 2023
Merged

DX: PsrAutoloadingFixerTest - do not build mock in data provider #7491

merged 2 commits into from Dec 2, 2023

Conversation

kubawerlos
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

coverage: 94.918%. remained the same
when pulling 7cce4f9 on 6b7562617765726c6f73:dx_PsrAutoloadingFixerTest
into 2ca2052 on PHP-CS-Fixer:master.

@kubawerlos kubawerlos enabled auto-merge (squash) December 2, 2023 14:44
Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

I remember that at some point Rector/ECS also moved away from SplFileInfo and utilised the string path. There's no point in passing around that object if we only need a path 👍.

@kubawerlos kubawerlos merged commit eb775b4 into PHP-CS-Fixer:master Dec 2, 2023
25 checks passed
@kubawerlos kubawerlos deleted the dx_PsrAutoloadingFixerTest branch December 2, 2023 17:21
@@ -382,7 +382,14 @@ final protected static function getTestFile(string $filename = __FILE__): \SplFi
{
static $files = [];

return $files[$filename] ?? $files[$filename] = new \SplFileInfo($filename);
return $files[$filename] ?? $files[$filename] = new class($filename) extends \SplFileInfo {
public function getRealPath(): string
Copy link
Member

Choose a reason for hiding this comment

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

why to override this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the failed tests in the 1st commit.

Copy link
Member

Choose a reason for hiding this comment

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

sorry but that is not the answer. It is basically "kerad, check it yourself" :(

if test from single test class is failing, because of custom logic that was originally isolated to given test class, i do not believe we should move that to affect test cases for all the rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i do not believe we should move that to affect test cases for all the rules

what if it affects it positive way?

Copy link
Member

Choose a reason for hiding this comment

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

it's affect in surprising way. we effectively override logic of built-in method to not return false on non-existing file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to not return false on non-existing file

non-existing, like a mock file?

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.

None yet

4 participants