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

[MigrateAtToConsecutiveExpectationsRector] Deprecate rule as way too ambiguous paths to upgrade and breaking, better handle manually #44

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Feb 2, 2022

After 3 hours of trying to fix the issue and looking at original PHPUnit issue with over 14 participants, sebastianbergmann/phpunit#4297, this Rector rule took too big piece of pie.

I checked the Moodle project that upgrade their tests - stronk7/moodle@e4f1c43, and there is many way to handle this upgrade. Most of them are:

// 51 changes in total
-$this->at(...);
+$this->once(...);

But there are couple other way, like:

// 8 changes
+$this->exactly();

or:

// 3 changes
+->withConsecutive(

Which is interesting, because the ->withConsecutive() was actually default solution for Rector, but here it's rather edge case.


There is no clear way to upgrade this A → B. We should leave this to the end-user and PHPStorm find + replace with fixing failing test case process. It's better to handle manually to avoid random breaks.

That's why this rule is removed.


Closes rectorphp/rector#6976, cc @umpirsky

@TomasVotruba TomasVotruba changed the title tv phpunit at Fix single expectation use case in MigrateAtToConsecutiveExpectationsRector Feb 2, 2022
@TomasVotruba TomasVotruba changed the title Fix single expectation use case in MigrateAtToConsecutiveExpectationsRector [MigrateAtToConsecutiveExpectationsRector] Deprecate rule as way too ambiguous paths to upgrade and breaking, better handle manually Feb 2, 2022
@TomasVotruba TomasVotruba force-pushed the tv-phpunit-at branch 6 times, most recently from 6bb2609 to a8cd024 Compare February 2, 2022 19:39
@umpirsky
Copy link
Contributor

umpirsky commented Feb 2, 2022

Too bad, but thanks for looking into it and posting detailed findings. 👍

@TomasVotruba TomasVotruba merged commit 49d312b into main Feb 2, 2022
@TomasVotruba TomasVotruba deleted the tv-phpunit-at branch February 2, 2022 20:36
@TomasVotruba
Copy link
Member Author

Thanks for understanding 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants