Re: [Wikitech-l] Declaring methods final in classes
On Sun, Sep 1, 2019 at 12:40 PM Aryeh Gregor wrote: > On Fri, Aug 30, 2019 at 10:09 PM Krinkle wrote: > > 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. [..] > > In my experience, classes that use a particular interface usually call > only one or two methods from it, which can usually be replaced by > returning fixed values or at most two or three lines of code. [..] It could sometimes not be worth it to mock for the reason you give, > but in my experience that's very much the exception rather than the rule. > [..] > Consider a class like ReadOnlyMode. It has an ILoadBalancer injected. > The only thing it uses it for is getReadOnlyReason(). When testing > ReadOnlyMode, we want to test "What happens if the load balancer > returns true? What about false?" A mock allows us to inject the > required info with a few lines of code. 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. > 5) In some cases you want to know that a certain method is being > called with certain parameters,[ ..] Maybe the bug changed the > WHERE clause to one that happens to select your row of test data > despite being wrong. > 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. > 6) [..] > One way to help catch inadvertent performance regressions is to test > that the means of ensuring performance are being used properly. For > instance, if a method is supposed to first check a cache and only fall > back to the database for a cache miss, we want to test that the > database query is actually only issued in the event of a cache miss. 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Fri, Aug 30, 2019 at 10:09 PM Krinkle wrote: > 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. In my experience, classes that use a particular interface usually call only one or two methods from it, which can usually be replaced by returning fixed values or at most two or three lines of code. The implementations of the stub methods usually can be very simple, because you know exactly the context that they'll be called and you can hard-code the results. It could sometimes not be worth it to mock for the reason you give, but in my experience that's very much the exception rather than the rule. > 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 :) Consider a class like ReadOnlyMode. It has an ILoadBalancer injected. The only thing it uses it for is getReadOnlyReason(). When testing ReadOnlyMode, we want to test "What happens if the load balancer returns true? What about false?" A mock allows us to inject the required info with a few lines of code. If you use a real load balancer, you're creating a number of problems: 1) Where do you get the load balancer from? You probably don't want to manually supply all the parameters. If you use MediaWikiServices, you're dependent on global state, so other tests can interfere with yours. (Unless you tear down the services for every test, and see next point.) 2) It's a waste of resources to run so much code just to generate "true" or "false". One reason unit tests are so much faster than integration tests is because they don't have to do all this setup of the services. 3) I want an ILoadBalancer whose getReadOnlyReason() will return true or false, so I can test how ReadOnlyMode will behave. How do I get a real one to do that? There's no setReadOnlyReason() in ILoadBalancer. Now I have to explore a concrete implementation and its dependencies to figure out how to get it to return "true" or "false". Then anyone reviewing my code has to do the same to verify that it's correct. This is instead of a few lines of code that are contained within my test file itself and are checkable for correctness via cursory inspection. 4) If the contract or implementation details of this totally separate interface/class change, it will break my test. Now anyone who changes LoadBalancer's implementation details risks breaking tests of classes that depend on it. Worse, they might make the tests incorrect but still pass. Perhaps I was setting readOnlyReason to true somehow, and now that way no longer works, so it's really returning false -- but maybe for some other reason my test now passes (perhaps other changes made at the same time). Probably nobody will ever notice until an actual bug arises. 5) In some cases you want to know that a certain method is being called with certain parameters, but it's nontrivial to check indirectly. For instance, suppose a certain method call is supposed to update a row in the database. Even if you didn't mind giving it a real database despite all the above reasons, how are you going to test the correct row is being updated? You'd have to populate an initial row (make sure the database gets reset afterwards!) and check that it was updated properly. But what if a refactor introduced a bug in the WHERE clause that happens not to stop the query from working in your case but might cause problems on production data? Maybe the bug changed the WHERE clause to one that happens to select your row of test data despite being wrong. Trying to test every possible set of preexisting data is not feasible. What's extremely simple is just testing that the expected query is issued to a mock. 6) Performance is important to us. The direct way to test performance is by measuring test run time, but this is unreliable because it's heavily affected by load on the test servers. Also, lots of performance issues only come up on large data sets, and we're probably not going to run tests routinely on databases the size of Wikipedia's. One way to help catch inadvertent performance regressions is to test that the means of ensuring performance are being used properly. For instance, if a method is supposed to first check a cache and only fall back to the database for a cache miss, we want to test that the database query
Re: [Wikitech-l] Declaring methods final in classes
On Thu, Aug 29, 2019 at 1:46 PM Aryeh Gregor wrote: > On Thu, Aug 29, 2019 at 1:02 AM Krinkle wrote: > > What did you want to assert in this test? > > In a proper unit test, I want to completely replace all non-value > classes with mocks, so that they don't call the actual class' code. > This way I can test the class under test without making assumptions > about other classes' behavior. > > 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Thu, Aug 29, 2019 at 5:30 PM Daniel Kinzler wrote: > 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. Well, for classes that aren't supposed to be subclassed at all, we could mark them final. :) ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
Am 29.08.19 um 15:31 schrieb Adam Wight: > This is a solution I'd love to see explored. Subclassing is now considered > harmful, and it would be possible to refactor existing cases to use > interfaces, traits, composition, etc. 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Wed, Aug 28, 2019 at 4:29 PM Daniel Kinzler wrote: > Subclassing should be very limited anyway, and even more limited across > module > boundaries [...] This is a solution I'd love to see explored. Subclassing is now considered harmful, and it would be possible to refactor existing cases to use interfaces, traits, composition, etc. -Adam ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
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 ha scritto: > On Thu, Aug 29, 2019 at 1:02 AM Krinkle wrote: > > What did you want to assert in this test? > > In a proper unit test, I want to completely replace all non-value > classes with mocks, so that they don't call the actual class' code. > This way I can test the class under test without making assumptions > about other classes' behavior. > > This is not possible at all if any method is declared final. As soon > as the class under test calls a final method, you cannot mock the > object. This is without any use of expects() or with() -- even just > method()->willReturn(). > > > I find there is sometimes a tendency for a test to needlessly duplicate > the > > source code by being too strict on expecting exactly which method is > called > > to the point that it becomes nothing but a more verbose version of the > > source code; always requiring a change to both. > > > > Personally, I prefer a style of testing where it providers a simpler view > > of the code. More like a manual of how it should be used, and what > > observable outcome it should produce. > > The idea of good unit tests is to allow refactoring without having to > worry too much that you're accidentally changing observable behavior > in an undesired way. Ideally, then, any observable behavior should be > tested. Changes in source code that don't affect observable behavior > will never necessitate a change to a test, as long as the test doesn't > cheat with TestingAccessWrapper or such. > > This includes tests for corner cases where the original behavior was > never considered or intended. This is obviously less important to test > than basic functionality, but in practice, callers often accidentally > depend on all sorts of incidental implementation details. Thus > ideally, they should be tested. If the test needs to be updated, that > means that some caller somewhere might break, and that should be taken > into consideration. > > On Thu, Aug 29, 2019 at 1:12 AM Aaron Schulz > wrote: > > Interfaces will not work well for protected methods that need > > to be overriden and called by an abstract base class. > > If you have an interface that the class implements, then it's possible > to mock the interface instead of the class, and the final method > problem goes away. Of course, your "final" is then not very useful if > someone implements the interface instead of extending the class, but > that shouldn't happen if your base class has a lot of code that > subclasses need. > > On Thu, Aug 29, 2019 at 10:37 AM Daniel Kinzler > wrote: > > Narrow interfaces help with that. If we had for instance a cache > interface that > > defined just the get() and set() methods, and that's all the code needs, > then we > > can just provide a mock for that interface, and we wouldn't have to > worry about > > WANObjectCache or its final methods at all. > > Any interface would solve the problem, even if it was just a copy of > all the public methods of WANObjectCache. That would be inelegant, but > another possible solution if we want to keep using final. > > ___ > Wikitech-l mailing list > Wikitech-l@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Thu, Aug 29, 2019 at 1:02 AM Krinkle wrote: > What did you want to assert in this test? In a proper unit test, I want to completely replace all non-value classes with mocks, so that they don't call the actual class' code. This way I can test the class under test without making assumptions about other classes' behavior. This is not possible at all if any method is declared final. As soon as the class under test calls a final method, you cannot mock the object. This is without any use of expects() or with() -- even just method()->willReturn(). > I find there is sometimes a tendency for a test to needlessly duplicate the > source code by being too strict on expecting exactly which method is called > to the point that it becomes nothing but a more verbose version of the > source code; always requiring a change to both. > > Personally, I prefer a style of testing where it providers a simpler view > of the code. More like a manual of how it should be used, and what > observable outcome it should produce. The idea of good unit tests is to allow refactoring without having to worry too much that you're accidentally changing observable behavior in an undesired way. Ideally, then, any observable behavior should be tested. Changes in source code that don't affect observable behavior will never necessitate a change to a test, as long as the test doesn't cheat with TestingAccessWrapper or such. This includes tests for corner cases where the original behavior was never considered or intended. This is obviously less important to test than basic functionality, but in practice, callers often accidentally depend on all sorts of incidental implementation details. Thus ideally, they should be tested. If the test needs to be updated, that means that some caller somewhere might break, and that should be taken into consideration. On Thu, Aug 29, 2019 at 1:12 AM Aaron Schulz wrote: > Interfaces will not work well for protected methods that need > to be overriden and called by an abstract base class. If you have an interface that the class implements, then it's possible to mock the interface instead of the class, and the final method problem goes away. Of course, your "final" is then not very useful if someone implements the interface instead of extending the class, but that shouldn't happen if your base class has a lot of code that subclasses need. On Thu, Aug 29, 2019 at 10:37 AM Daniel Kinzler wrote: > Narrow interfaces help with that. If we had for instance a cache interface > that > defined just the get() and set() methods, and that's all the code needs, then > we > can just provide a mock for that interface, and we wouldn't have to worry > about > WANObjectCache or its final methods at all. Any interface would solve the problem, even if it was just a copy of all the public methods of WANObjectCache. That would be inelegant, but another possible solution if we want to keep using final. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Thu, Aug 29, 2019 at 9:36 AM Daniel Kinzler wrote: > > Narrow interfaces help with that. If we had for instance a cache interface > that > defined just the get() and set() methods, and that's all the code needs, > then we > can just provide a mock for that interface, and we wouldn't have to worry > about > WANObjectCache or its final methods at all. > > 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
> As such, to perhaps help with the conversation, I'd like to have a > practical example we can look at and compare potential solutions. Perhaps > from WANObjectCache, or perhaps with something else. Simetrical can speak on the concrete use case, but I'd like to give my thoughts on mocking WANObjectCache in general: when writing a unit test, the class under test should ideally be tested in isolation - that is, any of its dependencies should be mocked. If the dependency is inconsequential or there is a trivial implementation available, we don't have to be too strict about this. But complex dependencies should be avoided in unit tests, otherwise it's not a unit test but an integration test. WANObjectCache is 2600 lines of complex code. When testing something that has a WANObjectCache injected, we should inject a fake WANObjectCache, to preserve the level of isolation that makes the test a unit test. As long as WANObjectCache's most important methods, like get(), are final, this is impossible. I agree by the way that overly specific mocks go against the idea of a unit test. The test should test the contract, not the implementation. The mock should provide the minimum functionality needed by the code, it shouldn't assert things like "get() is called only once" - unless that is indeed part of the contract. Narrow interfaces help with that. If we had for instance a cache interface that defined just the get() and set() methods, and that's all the code needs, then we can just provide a mock for that interface, and we wouldn't have to worry about WANObjectCache or its final methods at all. -- Daniel Kinzler Principal Software Engineer, Core Platform Wikimedia Foundation ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
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 wrote: > On Tue, Aug 27, 2019 at 11:53 PM Daimona wrote: > > Personally, I don't like these limitations in PHPUnit and the like. IMHO, > > they should never be a reason for changing good code. > > 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. > > > And sometimes, methods have to be final. > > 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? > > > 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. > > 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. > > ___ > Wikitech-l mailing list > Wikitech-l@lists.wikimedia.org > https://lists.wikimedia.org/mailman/listinfo/wikitech-l -- -Aaron ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Tue, Aug 27, 2019 at 6:55 PM Aryeh Gregor wrote: > I see that in some classes, like WANObjectCache, most methods are declared > final. Why is this? [..] > > The problem is that PHPUnit mocks can't touch final methods. [..] What did you want to assert in this test? I find there is sometimes a tendency for a test to needlessly duplicate the source code by being too strict on expecting exactly which method is called to the point that it becomes nothing but a more verbose version of the source code; always requiring a change to both. Personally, I prefer a style of testing where it providers a simpler view of the code. More like a manual of how it should be used, and what observable outcome it should produce. I think PHPUnit's assertion helpers for things like method()->once() are quite useful. But, personally, I almost never need them. And in the few occasions where I did use them, it's never a private, static or final method (which can't be mocked). In some cases, I accidentally tried to mock such method and found every time it was either because of pre-existing technical debt, or because I misunderstood the code, or because I was testing arbitrary implementation details. As such, to perhaps help with the conversation, I'd like to have a practical example we can look at and compare potential solutions. Perhaps from WANObjectCache, or perhaps with something else. -- Timo ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
>> What benefits does it have to bind to a specific implementation that is > not >> guaranteed to stay as it is? > > If used properly, the final keyword should be for immutable > implementations. Otherwise, it could be a questionable use case. I think all the current uses of "final" in MW core are questionable, then... >> 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. > > It depends on how hard is it to set up the library. Ideally, we only have > to find a place where to enable it, nothing more. And I also think it won't > harm existing tests in any way. It doesn't even require a CI change. The > only possible drawback is performance / opcache integration pointed out > by @simetrical. Let me try to upload a test change and see how it goes, > tonight or tomorrow. 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
>> I agree, but it doesn't offer a strict guarantee. > > Do we need a strict guarantee more than we need unit tests? I think we need both. Or rather, we need unit tests more, but if that doesn't exclude using finals, we can have both. >> Why not just use final, then? > > 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. Ah yes, I'm actually writing with the idea in mind that we'll be able to set up such a tool. If finals and mocking were mutually exclusive, I'd also prefer to have better tests. >> But making methods mockable while also keeping the >> capability to use finals is even better IMHO. > > 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. Yes, agreed with that. >> In theory, for sure. But I believe there are lots of occurrences in our >> code where static methods are not pure functions. > > Which indeed is one of the things we are currently trying to fix, because static > code can't be mocked. Indeed, and that's not so bad because it (hopefully) forces people not to write non-pure static methods. > Two sides of a coin, I think. Each of them has its benefits and its > drawbacks, I'd say. > > What benefits does it have to bind to a specific implementation that is not > guaranteed to stay as it is? If used properly, the final keyword should be for immutable implementations. Otherwise, it could be a questionable use case. > 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. It depends on how hard is it to set up the library. Ideally, we only have to find a place where to enable it, nothing more. And I also think it won't harm existing tests in any way. It doesn't even require a CI change. The only possible drawback is performance / opcache integration pointed out by @simetrical. Let me try to upload a test change and see how it goes, tonight or tomorrow. Il giorno mer 28 ago 2019 alle ore 18:54 Daniel Kinzler < dkinz...@wikimedia.org> ha scritto: > Am 28.08.19 um 16:48 schrieb Daimona: > >> Subclassing should be very limited anyway, and even more limited across > > module > >> boundaries > > > > I agree, but it doesn't offer a strict guarantee. > > Do we need a strict guarantee more than we need unit tests? > > >> which could even be enforced via static analysis. > > > > Why not just use final, then? > > 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. > > >> 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. > > That's what we are trying to fix, and final stuff is making it hard. > > >> 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. > > 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. > > > In theory, for sure. But I believe there are lots of occurrences in our > > code where static methods are not pure functions. > > Which indeed is one of the things we are currently trying to fix, because > static > code can't be mocked. > > >> 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. > > What benefits does it have to bind to a specific implementation that is not > guaranteed to stay as it is? > > >> If I understand correctly, this would break as soon as the mock object > > hits a > >> type hint of instanceof check. That won't fly. > > > > No, that's only what happens with mockery. The tool I found just strips > > 'final' keywords from the PHP code - I believe, I still haven't looked at > > the implementation. > > 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 > > ___ > Wikitech-l mailing list >
Re: [Wikitech-l] Declaring methods final in classes
Am 28.08.19 um 16:48 schrieb Daimona: >> Subclassing should be very limited anyway, and even more limited across > module >> boundaries > > I agree, but it doesn't offer a strict guarantee. Do we need a strict guarantee more than we need unit tests? >> which could even be enforced via static analysis. > > Why not just use final, then? 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. >> 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. That's what we are trying to fix, and final stuff is making it hard. >> 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. 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. > In theory, for sure. But I believe there are lots of occurrences in our > code where static methods are not pure functions. Which indeed is one of the things we are currently trying to fix, because static code can't be mocked. >> 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. What benefits does it have to bind to a specific implementation that is not guaranteed to stay as it is? >> If I understand correctly, this would break as soon as the mock object > hits a >> type hint of instanceof check. That won't fly. > > No, that's only what happens with mockery. The tool I found just strips > 'final' keywords from the PHP code - I believe, I still haven't looked at > the implementation. 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 ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Wed, Aug 28, 2019 at 7:24 PM Lucas Werkmeister wrote: > As far as I can tell, it actually strips final tokens from *any* PHP file > that’s read, including by application code. Yes, but only if you turn it on, and we'd only turn it on for tests. > 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. I wonder how it interacts with an opcode cache. Is the cache going to return the cached result based on mtime or whatever, meaning you'll get a random mix of code with and without final and tests might fail because they got a cached version of the file that wasn't de-finalized? Or does it somehow know? (I don't see how it would.) I filed an issue on this: https://github.com/dg/bypass-finals/issues/12 Assuming it somehow works with an opcode cache, it shouldn't have to be a huge performance impact, because the files shouldn't be parsed often. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
> > No, that's only what happens with mockery. The tool I found just strips > 'final' keywords from the PHP code - I believe, I still haven't looked at > the implementation. 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 : > >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. > > No, that's only what happens with mockery. The tool I found just strips > 'final' keywords from the PHP code - I believe, I still haven't looked at > the implementation. > > > Il giorno mer 28 ago 2019 alle ore 16:29 Daniel Kinzler < > dkinz...@wikimedia.org> 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 > > > >
Re: [Wikitech-l] Declaring methods final in classes
>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. No, that's only what happens with mockery. The tool I found just strips 'final' keywords from the PHP code - I believe, I still haven't looked at the implementation. Il giorno mer 28 ago 2019 alle ore 16:29 Daniel Kinzler < dkinz...@wikimedia.org> 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@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
> 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. > 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. > 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. Il giorno mer 28 ago 2019 alle ore 09:30 Aryeh Gregor ha scritto: > On Tue, Aug 27, 2019 at 11:53 PM Daimona wrote: > > Personally, I don't like these limitations in PHPUnit and the like. IMHO, > > they should never be a reason for changing good code. > > 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. > > > And sometimes, methods have to be final. > > 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? > > > 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. > > 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. > > ___ > Wikitech-l mailing list > Wikitech-l@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
On Tue, Aug 27, 2019 at 11:53 PM Daimona wrote: > Personally, I don't like these limitations in PHPUnit and the like. IMHO, > they should never be a reason for changing good code. 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. > And sometimes, methods have to be final. 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? > 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. 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. ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
Hey, My understanding is that mocking final methods and types is a limitation that is not specific to just PHPUnit, or indeed to PHP itself. Mockery, another established PHP mock object framework, relies on a workaround for mocking final methods and types that prevents testing code with type constraints or instanceof checks[1]. On the JVM, the popular mocking framework Mockito similarly has to rely on instrumentation to provide support for this use case[2]. Personally, I do not have enough context to judge whether there is added value in declaring those methods in e.g. WANObjectCache as final. It may have been intended to explicitly prevent subclassing and overriding by extension code, although this is just a guess. A question that could be posed is whether these benefits are worth the tradeoff in terms of reduced testability. What do you think? —— [1] http://docs.mockery.io/en/latest/reference/final_methods_classes.html [2] https://static.javadoc.io/org.mockito/mockito-core/3.0.0/org/mockito/Mockito.html#39 Máté Szabó SOFTWARE ENGINEER +36 30 947 5903 WIKIA sp. z o.o. z siedzibą w Poznaniu, ul. Abp. A. Baraniaka 6 Sąd Rejonowy Poznań – Nowe Miasto i Wilda w Poznaniu, VIII Wydział Gospodarczy Krajowego Rejestru Sądowego, KRS 254365 NIP: 5252358778 Kapitał zakładowy: 50.000,00 złotych > On 27 Aug 2019, at 20:56, Daimona wrote: > > 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 ha scritto: > >> I see that in some classes, like WANObjectCache, most methods are declared >> final. Why is this? Is it an attempt to optimize? >> >> The problem is that PHPUnit mocks can't touch final methods. Any ->method() >> calls that try to do anything to them silently do nothing. This makes >> writing tests harder. >> >> If we really want these methods to be marked final for some reason, the >> workaround for PHP is to make an interface that has all the desired >> methods, have the class implement the interface, and make type hints all >> refer to the interface instead of the class. But if there's no good reason >> to declare the methods final to begin with, it's simplest to just drop it. >> ___ >> Wikitech-l mailing list >> Wikitech-l@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@lists.wikimedia.org > https://lists.wikimedia.org/mailman/listinfo/wikitech-l ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Declaring methods final in classes
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 ha scritto: > I see that in some classes, like WANObjectCache, most methods are declared > final. Why is this? Is it an attempt to optimize? > > The problem is that PHPUnit mocks can't touch final methods. Any ->method() > calls that try to do anything to them silently do nothing. This makes > writing tests harder. > > If we really want these methods to be marked final for some reason, the > workaround for PHP is to make an interface that has all the desired > methods, have the class implement the interface, and make type hints all > refer to the interface instead of the class. But if there's no good reason > to declare the methods final to begin with, it's simplest to just drop it. > ___ > Wikitech-l mailing list > Wikitech-l@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@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
