[policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests

I have a problem with this.

There is no proposal in this issue yet, only problem statement.

@daffie, I think you have misunderstood. This issue is only about how we handle backports of changes to the testing framework. Tests themselves are always considered internal and have been since before release. This policy does not propose to change that. It does not, in fact, propose anything yet, but making test classes public API certainly is not a part of it. So they can still of course be changed as needed, to extend test coverage or for whatever reason. Does that address your concern?

BTW it's mostly out of scope here, but I also disagree that we should make individual tests database-driver-specific other than tests for the drivers themselves; that would open the door for huge per-database regressions. Our tests should pass on all supported environments, and anything that causes a test to fail on one environment but not another is considered a critical bug. Core has per-commit and nightly testing on MySQL, Postgres, and SQLite with PHP 5.5, 5.6, and 7, of the same test suite. If there is a specific case where a test seems to be hardcoding MySQL expectations, @daffie, can you file an issue about that case to discuss it?

Let's keep this thread focused though on how, when, and whether to backport API changes to the testing framework (WTB, KTB, BTB, JSTB, etc.).

@Eric_A, can you provide other examples of API additions to BTB already being backported to 8.1.x following 8.1.0's release on April 20? I see #2735199: Convert web tests to browser tests for help module above, which is from a conversion (not an API addition) and so would normally be allowed by our policy since tests themselves are not API. (I actually have concerns about the "one module at a time" test conversions like that because it makes many things more complicated, including things like backports, but I've raised that on #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) already so we don't need to discuss here.) Under the current policy, the API addition issues are correctly being filed against 8.2.x only.

If it's already been happening and the branches have not already diverged, then it doesn't make sense to stop doing so now, and we should discuss whether to do so consistently, since the risk of disruption from API additions is hopefully low. Randomly backporting some, but not all, fixes is the worst of both worlds. So let's talk about the actual impacts of backporting the changes.

For example. Let's say I have a local test MyTestBase extends BrowserTestBase and the test has a local assertion MyTestBase::assertWebHeader(). If we add BrowserTestBase::assertWebHeader(), my test suddenly breaks, which is a sudden disruption for me, even though I was guaranteed disruptive fixes and internal API breaks would only occur with minor versions every six months. My test suite is suddenly failing, which could break deployments etc., and I have to spend an unknown amount of time to determine whether it's because of a regression in my code, then find out it was an unannounced and unscheduled API change, and then rename my own assertion in my tests, which then propagates that BC break downstream to any tests that subclass mine.

As @catch pointed out, this also happens to contrib authors with minor versions when we make (e.g.) a constructor change to add a service (an allowed internal API change in minor releases) -- unit tests which mock the class start to fail and have to be fixed. The release schedule is designed so that those disruptions can be planned for every six months, and fixed in batches when a new minor is coming out.

However, the added problem for tests is that, at least previously, contrib authors did not have control over which branch their contrib modules are tested against on DrupalCI. I just checked and it does appear to default to 8.2.x for contrib tests, which means those modules are already being disrupted by internal API changes on Drupal.org. (I can configure a custom branch test against 8.1.x with "custom parameters", but I'm not sure if that will be used nightly or not.) We've discussed in a DrupalCI testing policy issue (elsewhere) that contrib authors should be able to test against a stable release by default to avoid any of this.

But contrib authors are also only one audience potentially disrupted. Sites actually running 8.1.x and running test suites are another, as are custom modules being tested with internal systems instead of DrupalCI, and I think we'd need to consider carefully whether we want to start disrupting them every patch release as well.

There are benefits to backporting, such as keeping the branches from diverging to make bugfixes easier, but there are also risks. We have the semver-inspired release schedule and API change policy for a reason.

Thanks @Eric_A.

From what I remember @alexpott has stated a couple of times that (framework) testing changes were exempt from normal minor/patch level rules.

@alexpott was wrong about that, unfortunately; the testing framework itself has always been documented as being public API and there currently is no exception for it in the allowed changes policy. (Edit: "Testing improvements" was an exception to the pre-8.0.0 beta restrictions, which is probably where this confusion came from.) However, this issue should hopefully help clarify whether we do want to change toward such a policy (or some specific subset, i.e. we backport these kinds of API additions but not behavior changes/removals, or methods that exist on WTB are a reserved namespace for BTB too, or BTB is experimental, etc.), because there are different considerations than for production code.

@dawehner, I can totally understand how you feel; I actually really care about this initiative and I hate feeling like the documented best practice and the expectations of contributors pull me in opposite directions and force me to choose between them. I should have addressed this two months ago. Part of the reason we have policies is to try to prevent these sorts of confusions from arising -- if the policy turns out to be wrong or incomplete, as might be the case in this issue, then we should correct the policy as soon as possible rather than making small exceptions to it that only a few people actually know about.

It doesn't have anything to do with features though. In fact, almost the opposite. The allowed changes and API policies are intended to do as much as possible to make sure a patch-level update will never introduce a disruption so that we can build confidence and make sure people apply those updates immediately... even if they are doing something weird or unsupported. E.g. consider the 8.1.4/8.1.5/8.1.6 release series -- technically 8.1.4 should not have broken things for anyone, but it did. If we had not released hotfixes to resolve the problem for the sites affected, then it would have been even harder to get people to update to 8.1.7 for httpoxy. So that is why we try to err on the side of being overcautious for the production branch. Even if there is a 0.1% chance something breaks a site. And yeah, this adds maintainability overhead and, believe me, is just as frustrating for committers as it is for patch contributors. But we have been trying this way of creating releases for less than a year so we're bound to get things wrong from time to time.

Anyway, https://www.drupal.org/node/2696421 is the CI policy document. It came from an internal draft the DA did several rounds of feedback on, but without an issue. Adding to the summary.

By the way, following up on #Eric_A's feedback above, the last non-security commit to 8.1.x will happen on or before September 7, 2016; but the same questions/concerns will apply to 8.2.x, 8.3.x, etc. in the future.

Er. That was not intentional.

I posted #2797975: [policy] Nightly regression testing for full contributed projects against both the production and development minor branches which is relevant to the impact on contrib (not only for this policy discussion, but also even trivial changes in core minors that affect contrib tests).

So there are two problems here for me:

1. BTB is essentially experimental; it is not yet our "testing framework". We want to make it the primary framework for browser tests, but that is currently still simpletest until everything is stable enough to make the switch. So if we want feature parity for faster innovation on a pre-release, incomplete API, that's fine, but it comes with the cost of actually breaking people's BTB tests. Every time we make an API change in Migrate, for example, there are lots of screams, despite it being experimental and despite attempts to provide best-effort BC that isn't even required. The testing framework is an even more essential API.
2. I think backporting improvements to "the testing framework" is valuable. However, backporting improvements that actually do involve disruptions to the main extension point (once that is BTB) is much higher risk than fixing something in the test runner. This is why for other things we've tried to say we will make internal API additions in minors but not patch releases.

OTOH not backporting things because of differing test coverage, or backporting things without test coverage, are both undesirable as well. This is why we plan to schedule conversions to BTB for beta/RC phases -- to make the transition more smoothly.

@catch and @alexpott brought up the theoretical case where backporting a major bugfix gets blocked because a testing framework API addition is not available in the stable core branch.

I think it might make sense to backport things at maintainer discretion if that situation actually happens or is likely to happen. To use catch's example, if we have a critical issue that has a JS test that leverages an API addition to JSTB, when that happens, we know about it right away and can make a decision. On the other hand, if we backport a potential disruption purely for ease of maintenance, we don't know how many custom modules and deployments we might break, and have to rely on angry tweets after the fact -- or not knowing at all that we've ruined someone's day.

What I don't want is to open up loopholes to make API additions to patch releases for our convenience. Protected methods on BTB are kinda API (or at least disruptive to change), despite being protected, because BTB is 100% intended to be subclassed.

For #2763401: PHPunit browser tests should log all Mink requests, maybe there is a flaw in our API design here and these things should instead be services available to tests, similar to the new API for assertions.$this->assertSession()->food() (to use @catch's example) is going to be way safer and less likely to disrupt someone's existing test than $this->assertFood(). In the middle term where we are converting the test suite and using the legacy assertion API, we still have the problem, but maybe we can gradually phase that out over a couple minors since the legacy trait has been deprecated from the start and never supported API.

Our testing API is one of the most important APIs we provide. That means breaking BC or making disruptive changes is really bad, just as making it more useful and robust is really valuable.

So for #2763401: PHPunit browser tests should log all Mink requests, the consequence of not backporting it might be: You want to hit yourself in the face when you try to debug something on 8.2.x that you wrote a BTB for. Maybe you theoretically signed up for that pain when you used a new API instead of the stable legacy one, but it sure is annoying now.

The consequence of backporting it in its non-underscore form might be: Someone else's deployment breaks, because the darn API isn't actually marked as experimental and they had no idea we might disrupt it. Or less likely on an individual basis but statistically probable for the aggregate, the seemingly simple addition has some other disruptive consequence we didn't foresee. (I'm thinking of you, #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery.)

The strategy in #34 sounds really promising to me. (I wonder how many of our other backport woes could be solved if we stopped avoiding private methods so much?)

So thinking about this. Increased visibility is a kind of API addition. So if we increase the visibility from nonexistent to private to protected to public even, each time, it's still an API addition, so maybe there's no reason not to make things private at first. It won't break subclasses. And since a private method definitely cannot be confused with public API, it also does not violate semver in principle.

Is that really better? Why would a new method on assertSession() not break stuff the same as a new method on BTB?

Well I think we were thinking that the subclasses would have their own assertions on their own assertion object, instantiated separately. Edit: or even straight on the class itself if it's actually coupled to that test. They would not be adding anything to WebAssert or directly extending it, just using the instantiated one. So there would never be a name collision. Or does that not make sense?