- Issue created by @nicxvan
- π¬π§United Kingdom catch
For me there are three reasons not to have tests that only test deprecation messages.
1. We increasingly require deprecations (instead of direct changes/removal) for code that is technically @internal - 'best effort' deprecations for constructor arguments, protected methods on a controller etc.
It is quite low-effort to add @trigger_error() and @deprecated for those cases, adding a test too would make it more onerous when the deprecation itself isn't strictly required.
If we required deprecations for non-internal code but not for internal code, then it adds a lot of overhead figuring out whether a specific method counts as internal or not - especially since we rely on the bc policy docs more than @internal/@api.
The best-effort deprecations have been shown to reduce pain for contrib because we know people often rely on @internal code even when we advise against it. Testing of deprecation methods has not yet been missed. I would not feel as comfortable asking for 'best effort' deprecations if that also meant asking for test coverage.
2.When we used to add deprecation testing, especially around 9.4/9.5/10.0, I think several times I was committing a patches to Drupal 10 adding a 9.x deprecation + test so it could be backport to 9.x as-is, then later, sometimes the same day/week, committing a patch to Drupal 10 to remove the deprecation and test coverage again. This always felt like busy-work and not actually helping anyone. Even over a longer time-span it's still adding dozens/hundreds of tests to then delete them again.
3. Sometimes we update deprecation messages - versions, maybe changing change record links to point to a different CR. If that message is asserted in test coverage then everything has to be done in two places.
- πΊπΈUnited States nicxvan
I agree that we should not have those tests, I think they make more work than their value.
As a counterpoint, I think there are two things they could provide:
1. Ensure the deprecation was done correctly. I think this is taken care of by the code sniffing on the deprecations. the formatting has to be so specific I think you have to get it right.
2. It can make it easier to spot if someone removes a deprecation because they have to remove the test too. I almost didn't even put it here because if the reviewer and the committer missed that deprecation was removed they would probably miss a test being removed too so this is super edge case I think.I still land on the side that the policy that they do not exist is more beneficial, but wanted to mention the counterpoints I think I've seen.
- π³πΏNew Zealand quietone
@nicxvan, Thanks for adding suggested text, that makes it much easier to evaluate.
I think that the intent of the current text and the suggested text are the same, so why change it? Also, the suggested text uses 'must' which is not common in our policies, so I would need more time to consider it. And without the 'must' there is room for interpretation and to request a test in some, as yet unknown, situation. That is something I value in this situation. So, I don't yet see the need for a change.
And with my educator hat on, I think there are better solutions. Such as, to have the discussion on the issue, or in Slack. That can be valuable learning for the contributors commenting and for those reading. And, there is always the option to defer the question to a core committer.
- πΊπΈUnited States nicxvan
I agree that the intent is the same, but the current wording can be interpreted as it is not needed, but sometimes it can be added.
As in the issue summary I've been seeing some more issues with deprecations get a request for that test explicitly and seen it make more work or delay cause they then need to be removed before committing.
I did discuss this in slack and the line was interpreted very differently by someone else so I opened this to get clarity.
My main goal is to avoid issues waiting in needs review for a test being requested that isn't needed. I can always add it, but then it makes more work to remove it if the committers request it removed.
If the intention is to keep the current meaning as I interpreted then disambiguation would be helpful.
If it's intentional that it can sometimes be required then I can add the requested tests.