pharo-project
New driver for the push up method refactoring (P13)
Implements driver for handling the user interaction logic when applying the push up method refactoring. Tests are created for the new ReConditions Replaces PR#16438
Hi caro this is nice to have you back on that. @balsa-sarenac will certainly have a look. let us when you need feedback
Hi @Ducasse and @balsa-sarenac , I would appreciate your review on this PR. In particular I would like to discuss the 'fixable' and 'unfixable' breaking conditions that I explain in the video referred in the PR description
There are lots of failing tests:
[osx-64 / Tests-osx-64 / MacOSX64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.PharoDocComment.Tests/CommentTestCase/osx_64___Tests_osx_64___nil/)
[osx-64 / Tests-osx-64 / MacOSX64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.PharoDocComment.Tests/CommentTestCase/osx_64___Tests_osx_64___nil_2/)
[osx-64 / Tests-osx-64 / MacOSX64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.PharoDocComment.Tests/CommentTestCase/osx_64___Tests_osx_64___nil_3/)
[osx-64 / Tests-osx-64 / MacOSX64.UnifiedFFI.Tests.FFIAutoReleaseOptionCalloutTest.testExternalStructure](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.UnifiedFFI.Tests/FFIAutoReleaseOptionCalloutTest/osx_64___Tests_osx_64___testExternalStructure/)
[osx-64 / Tests-osx-64 / MacOSX64.UnifiedFFI.Tests.FFIAutoReleaseOptionCalloutTest.testVoidPointer](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.UnifiedFFI.Tests/FFIAutoReleaseOptionCalloutTest/osx_64___Tests_osx_64___testVoidPointer/)
[unix-64 / Tests-unix-64 / Unix64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/Unix64.PharoDocComment.Tests/CommentTestCase/unix_64___Tests_unix_64___nil/)
[unix-64 / Tests-unix-64 / Unix64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/Unix64.PharoDocComment.Tests/CommentTestCase/unix_64___Tests_unix_64___nil_2/)
[unix-64 / Tests-unix-64 / Unix64.PharoDocComment.Tests.CommentTestCase.nil](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/Unix64.PharoDocComment.Tests/CommentTestCase/unix_64___Tests_unix_64___nil_3/)
[osx-64 / Tests-osx-64 / MacOSX64.Collections.Sequenceable.Tests.ArrayTest.testShuffleBy](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Collections.Sequenceable.Tests/ArrayTest/osx_64___Tests_osx_64___testShuffleBy/)
[osx-64 / Tests-osx-64 / MacOSX64.Collections.Support.Tests.CollectionTest.testAtRandomWeighting](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Collections.Support.Tests/CollectionTest/osx_64___Tests_osx_64___testAtRandomWeighting/)
[osx-64 / Tests-osx-64 / MacOSX64.Collections.Support.Tests.CollectionTest.testAtRandomWeightingMultiple](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Collections.Support.Tests/CollectionTest/osx_64___Tests_osx_64___testAtRandomWeightingMultiple/)
[osx-64 / Tests-osx-64 / MacOSX64.Kernel.Extended.Tests.UnicodeTest.testRNG](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Kernel.Extended.Tests/UnicodeTest/osx_64___Tests_osx_64___testRNG/)
[osx-64 / Tests-osx-64 / MacOSX64.Network.Tests.UUIDPrimitivesTest.testCreationNodeBased](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Network.Tests/UUIDPrimitivesTest/osx_64___Tests_osx_64___testCreationNodeBased/)
[osx-64 / Tests-osx-64 / MacOSX64.RTree.Tests.RTreeTest.testRemoveLeaf1](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.RTree.Tests/RTreeTest/osx_64___Tests_osx_64___testRemoveLeaf1/)
[osx-64 / Tests-osx-64 / MacOSX64.Random.Tests.RandomTest.testDistribution](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Random.Tests/RandomTest/osx_64___Tests_osx_64___testDistribution/)
[osx-64 / Tests-osx-64 / MacOSX64.Random.Tests.RandomTest.testPrimitiveRandomGeneration1](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Random.Tests/RandomTest/osx_64___Tests_osx_64___testPrimitiveRandomGeneration1/)
[osx-64 / Tests-osx-64 / MacOSX64.Random.Tests.RandomTest.testPrimitiveRandomGeneration2](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Random.Tests/RandomTest/osx_64___Tests_osx_64___testPrimitiveRandomGeneration2/)
[osx-64 / Tests-osx-64 / MacOSX64.Random.Tests.RandomTest.testPrimitiveRandomGeneration3](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Random.Tests/RandomTest/osx_64___Tests_osx_64___testPrimitiveRandomGeneration3/)
[osx-64 / Tests-osx-64 / MacOSX64.Roassal.Global.Tests.RSForceBasedLayoutTest.testAddNodesAndEdges](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Roassal.Global.Tests/RSForceBasedLayoutTest/osx_64___Tests_osx_64___testAddNodesAndEdges/)
[osx-64 / Tests-osx-64 / MacOSX64.Roassal.Global.Tests.RSRTreeTest.testRemoveLeaf1](https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-16637/6/testReport/junit/MacOSX64.Roassal.Global.Tests/RSRTreeTest/osx_64___Tests_osx_64___testRemoveLeaf1/)
Related breaking tests are due to modifications to RBPullUpMethodRefactoring preconditions and breaking changes. I am checking the best way to solve the issues without modifying tests
@carolahp I would like to understand the difference between an unfixable case and a precondition that is violated. To me if the case is unfixable then it means that we should not apply the refactoring.
@carolahp I would like to understand the difference between an unfixable case and a precondition that is violated. To me if the case is unfixable then it means that we should not apply the refactoring.
@Ducasse the only difference is that an unfixable case is associated with a choice, for example "browse overriden method". However, I didn't think before that it may be simpler to just make these "unfixable cases" into preconditions.
I will update the PR to do this
Hey Caro, great work! Hello @balsa-sarenac , thanks :)
For the question on "fixable" vs "unfixable". I agree with Steph that "unfixable"s are basically applicability preconditions. And for breaking changes, we know this from the beginning that some of them can be fixed by other refactoring (what you refer to as "fixable") and others are just warnings (the refactoring cannot promise behaviour preservation and there are no fixes it can offer either). We didn't had them so far in the architecture since there was not a need to differentiate them. Maybe they can just be methods on preconditions, that if a precondition fail you can ask it what is the refactoring that fixes you? However, I see a drawback here with reusability (multiple refactorings use the same precondition and they want different fixes when they fail, or for one refactoring there can be breaking change and for other is applicability). Let me know how did you planned to integrate this in the current architecture? After all the modifications we introduced the 3 breaking change preconditions we have left (referenced instance and shared variables, and duplicated method in sibling) can all the fixed applying additional transformations. I never thought of the reusability issue you propose, but I agree that this is a problem that must be solved by the architecture.
While I get your new review I will work on a new driver (probably ReMoveMethodsToClassSide) and pay attention to this issue
I am thinking on renaming the driver and all UI related text from "push up" to "pull up". Do you think I should I do this in a new PR?
I will integrate it. I will check in Martin Fowler 's book how the refactoring is named. BTW it would be good to have (R) in the menu to follow the convention.
Are you sure the failing tests are not related? It is hard to me as we now have 76 (!) failing tests. Just checking that there is not one more than the one we know are failing is too hard for me to do and this is why I am not merging anything until we have the tests in a shape that I can be sure that the current PR dow not add even more failures
@MarcusDenker @Ducasse I am not sure what to do, given the current state of tests, do you advice me to I open a new PR with these changes and recheck for the cause of failing tests? Or should I wait for the moment?
Caro, you can try to fix any refactoring related failing tests (push up method) from the Pharo13 branch on a new branch.
