Personally, I don't like these limitations in PHPUnit and the like. IMHO, they should never be a reason for changing good code. And sometimes, methods have to be final. I don't know about the specific case, though. Anyway, some time ago I came across [1], which allows mocking final methods and classes. IIRC, it does that by removing the `final` keywords from the tokenized PHP code. I don't know how well it works, nor if it could degrade performance, but if it doesn't we could bring it in via composer. [1] - https://github.com/dg/bypass-finals Il giorno martedì 27 agosto 2019, Aryeh Gregor <ayg(a)aryeh.name> ha scritto: -- https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy "Daimona" is not my real name -- he/him

On Tue, Aug 27, 2019 at 11:53 PM Daimona <daimona.wiki(a)gmail.com> wrote: I don't like these limitations either, but testing is an integral part of development, and we need to code in a way that facilitates testing. In each case we need to make a cost-benefit analysis about what's best for the project. The question is whether there's any benefit to using final that outweighs the cost to testability. Why do methods ever "have" to be final? Someone who installs an extension accepts that they get whatever behavior changes the extension makes. If the extension does something we don't want it to, it will either work or not, but that's the extension's problem. This is exactly the question: why do we ever want methods to be final? Is there actually any benefit that outweighs the problems for testing? That would be a nice solution if it works well. If someone wants to volunteer to try to get it working, then we won't need to have this discussion. But until someone does, the question remains.

I see no good use for final methods or classes. Or rather: I see a very limited benefit and a pretty massive cost. Subclassing should be very limited anyway, and even more limited across module boundaries, which could even be enforced via static analysis. Method contracts should be enforced by compliance tests. When following these principles, making methods and classes final has little benefit. Preventing mocking is however a pretty massive cost. I'd just remove the final markers. But maybe I'm overlooking something?... Am 28.08.19 um 10:38 schrieb Daimona: Static code should only be used for pure functions. That's a very limited use case. This should eb done by documenting contracts, and having tests ensure compliance with these contracts. Final methods make callers rely on a specific implementation, which may still end up changing anyway. If I understand correctly, this would break as soon as the mock object hits a type hint of instanceof check. That won't fly. -- Daniel Kinzler Principal Software Engineer, Core Platform Wikimedia Foundation

As far as I can tell, it actually strips final tokens from *any* PHP file that’s read, including by application code. It seems to override the standard PHP handler for the file:// protocol, and rely on the fact that the PHP engine also uses that handler to read source code files. (Also, I think it only applies to .php files and not Apache-style .php.en files, but I guess that’s not a problem for us.) Am Mi., 28. Aug. 2019 um 16:49 Uhr schrieb Daimona &lt;daimona.wiki(a)gmail.com&gt;om>: > >Subclassing should be very limited anyway, and even more limited across > module > >boundaries > > I agree, but it doesn't offer a strict guarantee. > > > which could even be enforced via static analysis. > > Why not just use final, then? > > > Method contracts should be enforced by compliance tests. When following > these principles, making > > methods and classes final has little benefit. > > Ideally, yes. But I don't think our codebase has enough tests for that. > > > Preventing mocking is however a pretty massive cost. > > Definitely yes. But making methods mockable while also keeping the > capability to use finals is even better IMHO. > > > Static code should only be used for pure functions. That's a very limited > use case. > > In theory, for sure. But I believe there are lots of occurrences in our > code where static methods are not pure functions. > > >> If you want to make sure that any subclass won't ever change the > >> implementation of a method, and thus all callers know what to expect > from > >> calling a final method. > >> I see finals as a sort of safeguard to help write better code, like e.g. > >> typehints. > > > >This should eb done by documenting contracts, and having tests ensure > compliance > >with these contracts. Final methods make callers rely on a specific > >implementation, which may still end up changing anyway. > > Two sides of a coin, I think. Each of them has its benefits and its > drawbacks, I'd say. > > >> IMHO this would be a perfect compromise. I've filed T231419 for that, > and I > >> also think that before discussing any further, we should try to see if > we > >> can install that tool. > > > >If I understand correctly, this would break as soon as the mock object > hits a > >type hint of instanceof check. That won't fly. > > > Il giorno mer 28 ago 2019 alle ore 16:29 Daniel Kinzler < > dkinzler(a)wikimedia.org&gt; ha scritto: > > > I see no good use for final methods or classes. Or rather: I see a very > > limited > > benefit and a pretty massive cost. > > > > Subclassing should be very limited anyway, and even more limited across > > module > > boundaries, which could even be enforced via static analysis. Method > > contracts > > should be enforced by compliance tests. When following these principles, > > making > > methods and classes final has little benefit. Preventing mocking is > > however a > > pretty massive cost. > > > > I'd just remove the final markers. But maybe I'm overlooking > something?... > > > > Am 28.08.19 um 10:38 schrieb Daimona: > > >> I don't like these limitations either, but testing is an integral part > > >> of development, and we need to code in a way that facilitates testing. > > > > > > This is especially true for e.g. static methods, but here we'd be > > > renouncing to a possibly useful feature. > > > > Static code should only be used for pure functions. That's a very limited > > use case. > > > > >> Why do methods ever "have" to be final? > > > > > > If you want to make sure that any subclass won't ever change the > > > implementation of a method, and thus all callers know what to expect > from > > > calling a final method. > > > I see finals as a sort of safeguard to help write better code, like > e.g. > > > typehints. > > > > This should eb done by documenting contracts, and having tests ensure > > compliance > > with these contracts. Final methods make callers rely on a specific > > implementation, which may still end up changing anyway. > > > > > > > >> That would be a nice solution if it works well. If someone wants to > > >> volunteer to try to get it working, then we won't need to have this > > >> discussion. But until someone does, the question remains. > > > > > > IMHO this would be a perfect compromise. I've filed T231419 for that, > > and I > > > also think that before discussing any further, we should try to see if > we > > > can install that tool. > > > > If I understand correctly, this would break as soon as the mock object > > hits a > > type hint of instanceof check. That won't fly. > > > > > > -- > > Daniel Kinzler > > Principal Software Engineer, Core Platform > > Wikimedia Foundation > > > > _______________________________________________ > > Wikitech-l mailing list > > Wikitech-l(a)lists.wikimedia.org > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > > > -- > https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy > "Daimona" is not my real name -- he/him > _______________________________________________ > Wikitech-l mailing list > Wikitech-l(a)lists.wikimedia.org > https://lists.wikimedia.org/mailman/listinfo/wikitech-l -- Lucas Werkmeister (he/er) Full Stack Developer Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin Phone: +49 (0)30 219 158 26-0 https://wikimedia.de Imagine a world in which every single human being can freely share in the sum of all knowledge. Help us to achieve our vision! https://spenden.wikimedia.de Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V. Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer 27/029/42207.

Am 28.08.19 um 16:48 schrieb Daimona: Do we need a strict guarantee more than we need unit tests? Because it makes it impossible to write unit tests. Maybe not impossible with the tool you pointed to. If that thing works, it becomes: it requires effort to set up the CI infrastructure to allow this to work, and we don't know who is going to do that, or when. That's what we are trying to fix, and final stuff is making it hard. If that can be made to work, sure. I'm just saying that an inability to mock far outweights the potential benefits of declaring things as final. Which indeed is one of the things we are currently trying to fix, because static code can't be mocked. What benefits does it have to bind to a specific implementation that is not guaranteed to stay as it is? If somebody is volunteering to do the necessary work in the CI infrastructure, fine. To me it just seems like just removing the final modifier is the easier and cheaper solution, and doesn't have any big downside. -- Daniel Kinzler Principal Software Engineer, Core Platform Wikimedia Foundation

I think all the current uses of "final" in MW core are questionable, then... Cool, thank you for looking into this! Sorry if I sounded very negative. I'm trying to be pragmatic and avoid extra effort where it is not needed. I appreciate your help with evaluating this! -- Daniel Kinzler Principal Software Engineer, Core Platform Wikimedia Foundation

Well, changing something in core and breaking a production extenison doing something silly can't be waived away with "it's the extension's problem" ;) I mostly use "final" to enforce a delegation pattern, where only certain key bits of functionality should be filled in by subclasses. It mostly comes out of years and years of bad experience with core and extension code subclassing things in annoying ways that inevitably have to be cleaned up as a side-task to getting some other feature/refactoring patch to pass CI. It's a clear way to both document and enforce subclass implementation points. The only reason not to use it is for tests, and I have removed "final" before (placed in BagOStuff) when I couldn't come up with another workaround. Interfaces will not work well for protected methods that need to be overriden and called by an abstract base class. If no PHP/PHPUnit fix is coming soon, as a practical matter, I'm sure some other alternative documentation and naming style pattern could be standardized so that people actually follow it and don't make annoying and fragile dependencies. On Wed, Aug 28, 2019 at 12:30 AM Aryeh Gregor &lt;ayg(a)aryeh.name&gt; wrote: -- -Aaron

On Thu, Aug 29, 2019 at 9:36 AM Daniel Kinzler &lt;dkinzler(a)wikimedia.org&gt; wrote: I think this is the right solution, forbidding one feature of the language (final) because of the current design of WANObjectCache seems going too far in my opinion. -- David Causse

Note that https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/533067/ is now ready for review, and works as expected. That could make this discussion unnecessary. Il giorno gio 29 ago 2019 alle ore 14:46 Aryeh Gregor &lt;ayg(a)aryeh.name&gt; ha scritto: -- https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy "Daimona" is not my real name -- he/him

Am 29.08.19 um 15:31 schrieb Adam Wight: I wouldn't say subclassing is considered harmful in all cases. In fact, for extension points, subclassing is preferred over directly implementing interfaces. But subclassing should be used very carefully. It's the most tight form of coupling. It should be avoided if an alternative is readily available. But subclassing across module boundaries should be restricted to classes explicitly documented to act as extension points. If we could enforce this automatically, that would be excellent. -- Daniel Kinzler Principal Software Engineer, Core Platform Wikimedia Foundation

On Thu, Aug 29, 2019 at 1:46 PM Aryeh Gregor &lt;ayg(a)aryeh.name&gt; wrote: Using createMock() for an interface or class makes sense to me mostly for external dummies that are requires for some reason (an anti-pattern that I think should be avoided. If such a dependency is truly optional and of the fire-and-forget type, it should have a no-op implementation that is used by default, like we do with Loggers. For anything else, it doesn't really work in my experience because PHPUnit won't actually provide a valid implementation of the interface. It returns null for anything, which is usually not a valid implementation of the contract the class under test depends on. As a result, you end up with hundreds of lines of phpunit-mockery to basically reimplement the dependency's general functionality from scratch, every time. I have seen this too many times and fail to see the added value for all that complexity and maintenance, compared to simply letting the actual class do its work. Practically speaking, what kind of problem would that test methodology catch? I believe others may have a valid answer to this, but I haven't yet seen it. Perhaps it's a source of problems I tend to prevent by other means. There's more than one way to catch most problems :) You can draw from that that my definition of "unit" and "integration" are probably different from yours. To me a unit is a single class, but I think it's fine to be given other class objects. I see each unit an isolated subtree, a controlled environment where all dependencies are known, explicitly specified, injected, and have no side-effects after the test is done apart from within the objects you created for that test. -- Timo

On Sun, Sep 1, 2019 at 12:40 PM Aryeh Gregor &lt;ayg(a)aryeh.name&gt; wrote: It could sometimes not be worth it to mock for the reason you give, While I do prefer explicit dependencies, an extreme of anything is rarely good. For this case, I'd agree and follow the same approach. And I think there are plenty of other examples where I'd probably go for a partial mock. Doing so feels like a compromise to me, as I have often seen this associated with working around technical debt (e.g. the relationship between those classes could've been better perhaps). But I think it's fair to say there will be cases where the relationship is fine and it still makes sense to write a partial mock to keep the test simple. In addition to that, I do think it is important to also have at least one test case at the integration level to verify that the higher-level purpose and use case of the feature works as intended - where you'd have to construct the full dependency tree and mock or stub only the signal that is meant to travel through the layers. Thus ruling out any mistake in the (separate) unit tests for ReadOnlyMode and LoadBalancer. But, you wouldn't need to have full coverage through that - just test the communication between the two. The unit tests would then aim for coverage of all the edge cases and variations. Thanks for writing this up. Yep, this is important. I tend to go for lower level observation instead of injected assertions. E.g. mock IDatabase::select/insert etc. to stash each query in an array, and then toward the end assert that the desired queries have occurred exactly as intended and nothing more. Similar to how one can memory-buffer a Logger instance. I like the higher confidence this gives and I find it easier to write than a PHPUnit mock where each function call is precisely counted and params validated, while being more tolerant to internal details changing over time. Having said that, they both achieve the same goal. I suppose its a matter of taste. Certainly nothing wrong with the PHPUnit approach, I'd just not do it myself as much. Thanks for reminding me of this. Yep, that works. I tend to do this differently, but it works and its important that we make these assertions somewhere. I don't disagree :) -- Timo