Support deprecation testing for multiple branches on contributed modules on Drupal.org

Adding in at least some of this flexibility is what DrupalCI issue #2951375: Allow for build targets is about.

#3001958: 4 test fails due to using deprecated code on 8.6 and 8.7 since #2996789: temporarily fork the test trait is the contrib project issue that triggered this. It worked around the problem by duplicating/forking core code into the contrib module so that it could pass tests on 8.5, 8.6 and 8.7 with deprecation testing turned on.

What if we treat deprecation messages similar to coding style violations?

We report them separately, additionally, specifically *changes* to the branch test on patches. So you can see it, but deprecations would not fail your test.

> could pass tests on 8.5, 8.6 and 8.7 with deprecation testing turned on.

Because there should not be a requirement to do this. Deprecations are by design introduced in arbitrary minor versions and you can only use the new stuff if you no longer support the old ones.

Even more so now with the new security policy of supporting two minors.

Ideally we'd have a way to report deprecations without failing, but we don't.

I still think we should have more flexibility in drupalci, but some conversations on slack are bringing me around to the idea that contrib shouldn't concern itself with deprecations other than when they show up in the lowest supported core version. Theoretically, contrib should never have to change based on deprecations until preparing for D9.

Also, if we had proper semver you could just make a new branch, target it to core 8.5.x (instead of 8.x), release it as jsonapi 8.x-8.5.0, and be done. #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc)

> Ideally we'd have a way to report deprecations without failing, but we don't.

I know we don't, I'm just saying that it might be easier to build a system like the one we have for coding style violations instead of a complex matrix based test system, not least because people like to just test everything with everything then.

> Also, if we had proper semver you could just make a new branch, target it to core 8.5.x (instead of 8.x), release it as jsonapi 8.x-8.5.0, and be done.

It's trivial to release jsonapi-8.x-1.whatever and combine that with a dependency on 8.5.x. Works with both composer dependency as well the version checks of Drupal core. The only thing you can't do at the moment is require e.g. 8.5.3. And 8.x-1.2 is already exposed to composer as 1.2.0

> are bringing me around to the idea that contrib shouldn't concern itself with deprecations other than when they show up in the lowest supported core version

Yep. Which is interesting too, in a way. All our deprecation messages contain the version. What if we'd somehow add an environment variable that can be set somehow that would filter out deprecation messages of later versions. Could even be based on the existing version information that we have of a project. So if module X doesn't declare an explicit version dependency on core, we'd not report any deprecation messages, and if it says "drupal:system (>=8.5)", like pathauto for example does, we could say that we can report deprecation messages except those that contain 8.6.x or higher.

All our deprecation messages contain the version.

This is an assumption you might want to check. :-)

What if we'd somehow add an environment variable that can be set somehow that would filter out deprecation messages of later versions.

We could theoretically do a @deprecated diff between releases, store deprecation error messages in a lookup table, and use that database to filter for targeted core based on info file. Even that analysis would be broken because not all error messages are static, and the mapping between @deprecation and @trigger_error() is not 1:1 even when they're both present.

Or, we could add a way to surface deprecation messages without failing tests by adding something like run-tests: suppress-deprecations: report-only to drupalci.yml. This puts E_USER_DEPRECATION in the realm of coding standards, in terms of allowing maintainers to have different policies.

Seems like the latter is more doable.

It also helps us figure out a way to say that a contrib module is 'D9-ready', because d.o could run all the tests to a spec and then add a banner to projects that pass tests and don't report deprecations.

Theoretically, contrib should never have to change based on deprecations until preparing for D9.

That means contrib modules will not be ready when D9 ships, which is the opposite of the intent here.

I know we don't, I'm just saying that it might be easier to build a system like the one we have for coding style violations instead of a complex matrix based test system, not least because people like to just test everything with everything then.

+1

Yep. Which is interesting too, in a way. All our deprecation messages contain the version. What if we'd somehow add an environment variable that can be set somehow that would filter out deprecation messages of later versions. […] if it says "drupal:system (>=8.5)" […] we can report deprecation messages except those that contain 8.6.x or higher.

Oohhh!

Or, we could add a way to surface deprecation messages without failing tests […]

Oohhh!

It also helps us figure out a way to say that a contrib module is 'D9-ready', because d.o could run all the tests to a spec and then add a banner to projects that pass tests and don't report deprecations.

Yes! 🎉

This is a modification to DeprecationListenerTrait. It looks for an environmental variable called DRUPAL_PHPUNIT_DEPRECATIONS_ARE_WARNINGS and turns deprecations into warnings in the test results.

Warnings can be configured to fail a test run, but they normally don't.

Looking for some nice easy deprecations to log, I commented out the guardrails around @groups legacy within DeprecationListenerTrait::deprecationEndTest(). Then I ran PHPUnit like this:
DRUPAL_PHPUNIT_DEPRECATIONS_ARE_WARNINGS=true ./vendor/bin/phpunit -c core/ --testsuite unit --log-junit results.xml --group legacy

We see warnings like this when we run the test at the command line:

23) Drupal\Tests\user\Unit\PrivateTempStoreTest::testDelete
--- E_USER_DEPRECATION errors that could fail a test:
[*] \Drupal\user\PrivateTempStore is scheduled for removal in Drupal 9.0.0. Use \Drupal\Core\TempStore\PrivateTempStore instead. See https://www.drupal.org/node/2935639.
   1x in Drupal\Tests\user\Unit\PrivateTempStoreTest::testDelete

These would only show up in a DrupalCI run if you looked at the test artifacts, which isn't visible enough.

Luckily, in PHPUnit 6+, JUnit reports have a warning tag. (I'm pretty sure PHPUnit 5 or less doesn't.)

          <testcase name="testDelete" class="Drupal\Tests\user\Unit\PrivateTempStoreTest" classname="Drupal.Tests.user.Unit.PrivateTempStoreTest" file="/Users/paul/projects/drupal/core/modules/user/tests/src/Unit/PrivateTempStoreTest.php" line="263" assertions="6" time="0.026769">
            <warning type="PHPUnit\Framework\Warning">Drupal\Tests\user\Unit\PrivateTempStoreTest::testDelete
--- E_USER_DEPRECATION errors that could fail a test:
[*] \Drupal\user\PrivateTempStore is scheduled for removal in Drupal 9.0.0. Use \Drupal\Core\TempStore\PrivateTempStore instead. See https://www.drupal.org/node/2935639.
   1x in Drupal\Tests\user\Unit\PrivateTempStoreTest::testDelete

/Users/paul/projects/drupal/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:116
/Users/paul/projects/drupal/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:73
/Users/paul/projects/drupal/core/tests/Drupal/Tests/Listeners/DrupalListener.php:37
</warning>
          </testcase>

Then we can go over to the d.o integration and figure out a way to show warnings in additions to fails. We could have it look for the keyword 'E_USER_DEPRECATION', or define a deprecation warning class and filter by warning type.

And we'd also force DRUPAL_PHPUNIT_DEPRECATIONS_ARE_WARNINGS=true for all contrib tests, and make sure it's not set for core.

Note also that this patch will fail Drupal\Tests\datetime\Unit\Plugin\migrate\field\d6\DateFieldTest::testUnknownDateType because that method is annotated @runInSeparateProcess. Apparently somewhere between our process-isolated stuff and symfony's process-isolated stuff, we don't take that into account.

#13++

Related: https://github.com/symfony/symfony/issues/27936 + https://github.com/symfony/recipes/pull/442 + https://github.com/symfony/symfony/issues/28048 (on which @alexpott was pinged, cool!).

If we allow drupalci to move the file created at DRUPAL_EXPECTED_DEPRECATIONS_SERIALIZE path to build artifacts then we can add a coding violation kind of warning to the build result. This will allow us to enable the SYMFONY_DEPRECATIONS_HELPER by default for all projects.

Warnings can be configured to fail a test run, but they normally don't.

What a silly thing to say. :-) Tests don't fail, PHPUnit doesn't say, 'fail!', but it exits with 1.

Restating the use cases:

1) You can run core tests and you want tests to fail due to deprecations. That's the problem we've already solved for.

2) You can run contrib tests where you normally don't want to know about deprecations. We've already solved for not knowing. :-)

3) You can run contrib tests where you always find out about deprecations. Alter your branch's drupalci.yml to tell you. See https://cgit.drupalcode.org/examples/tree/drupalci.yml (Examples is a special case where we want to exemplify code, so it should always be up-to-date. Other contrib projects will have different BC needs.)

4) You want to run contrib tests where you know about deprecations but don't fail your branch tests or issues. We don't have this in a structured way, but you can make an issue that patches drupalci.yml and then run it from time to time and find out what you're missing.

So for case #4 we want to surface the information, much like we do CS and test fail info. You can do one click and find the results on a page like this: https://www.drupal.org/pift-ci-job/1082800

That page shows fails, and it also says "12 coding standards messages", although the link there is a little non-functional. Ideally, we'd have a similar notice: "35345 deprecation messages...." and you could click it and see some document about it.

In order to have a DrupalCI document about it, we have to make a log file artifact, or deduce one based on other log files. This is where I was headed in #11.

Currently, symfony/phpunit-bridge only gives us the ability to fail tests based on deprecations, and not the ability to log deprecations, whether or not they fail the tests. None of the issues in #15 look like they want to give us that.

#16 is on the right track, though there are problems: Mainly that we want SYMFONY_DEPRECATIONS_SERIALIZE instead, and the fact that SYMFONY_DEPRECATIONS_SERIALIZE only catches process-isolated tests:

            if ($this->willBeIsolated($test)) {
                $this->runsInSeparateProcess = tempnam(sys_get_temp_dir(), 'deprec');
                putenv('SYMFONY_DEPRECATIONS_SERIALIZE='.$this->runsInSeparateProcess);
            }

That's why I added to our test listener in #11, so we'd catch all tests, including plain-vanilla unit tests.

So if we have an upstream issue where we maybe add some kind of result to the Result object in a listener, which only optionally fails the test, and which adds that to the JUnit that's eventually exported, then we'll have something that meets our needs.

Otherwise we need #11 plus more better ideas. :-)

@Mile23
Is 4) about deprecations in vendor libraries? Because when I run a kernel test locally on the command line, I see for example the following:

./vendor/bin/phpunit --printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" modules/wip/feeds8/feeds/tests/src/Kernel/Feeds/Target/TimestampTest.php
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\feeds\Kernel\Feeds\Target\TimestampTest
.......                                                             7 / 7 (100%)

Time: 1.23 minutes, Memory: 6.00MB

OK (7 tests, 70 assertions)

Remaining deprecation notices (28)

  7x: \Drupal\Component\Utility\Unicode::strtolower() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Use mb_strtolower() instead. See https://www.drupal.org/node/2850048.
    6x in TimestampTest::testWithConfig from Drupal\Tests\feeds\Kernel\Feeds\Target
    1x in TimestampTest::testImportTimestamp from Drupal\Tests\feeds\Kernel\Feeds\Target

  7x: The "entity.query" service is deprecated. Use the 'entity_type.manager' service to get an entity type's storage object and then call \Drupal\Core\Entity\EntityStorageInterface::getQuery() or \Drupal\Core\Entity\EntityStorageInterface::getAggregateQuery() instead. See https://www.drupal.org/node/2849874
    6x in TimestampTest::testWithConfig from Drupal\Tests\feeds\Kernel\Feeds\Target
    1x in TimestampTest::testImportTimestamp from Drupal\Tests\feeds\Kernel\Feeds\Target

  7x: The Drupal\Core\Entity\Query\QueryFactory class is deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityStorageInterface::getQuery() or \Drupal\Core\Entity\EntityStorageInterface::getAggregateQuery() instead. See https://www.drupal.org/node/2849874.
    6x in TimestampTest::testWithConfig from Drupal\Tests\feeds\Kernel\Feeds\Target
    1x in TimestampTest::testImportTimestamp from Drupal\Tests\feeds\Kernel\Feeds\Target

  7x: drupal_set_message() is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931
    6x in TimestampTest::testWithConfig from Drupal\Tests\feeds\Kernel\Feeds\Target
    1x in TimestampTest::testImportTimestamp from Drupal\Tests\feeds\Kernel\Feeds\Target

Looks to me like it should somehow already be possible to grep these deprecation messages. And then upstream changes to Symfony/PHPUnit won't be absolutely necessary. I have little to no knowledge about DrupalCI, so maybe what I'm saying here is really hard to do.

I wrote a blog post about it https://www.previousnext.com.au/node/660.

RE #20: DrupalCI always runs contrib tests against the latest core branch. As a contrib module maintainer if I have to make a compatibility change for core minor version then I create a new release and add that to releases notes after the stable release of core minor version e.g. https://www.drupal.org/project/dynamic_entity_reference/releases/8.x-2.0.... I never have to create a new release for the core patch release at least till now but yes I don't know how would I name the new release if I ever have to do that but then again that's contrib semvar issue.

My point is contrib module current version should always support the core current supported branch(8.6.x) and tag a new release, if need be, as soon as the core release the new version till then keep all the latest core branch(8.7.x) changes in dev release. At least that's what I'm doing for the modules I maintain in contrib.

As a contrib module maintainer if I have to make a compatibility change for core minor version then [...]

Why do you have to?

There's always a BC layer, and if there isn't then that's a bug in core.

So you could, say, make an issue patch that turns on deprecation fails and run that patch against the previous core minor. Now you know your technical debt for your current release, so you can work on fixing that in your current dev.

That's good because current core dev's deprecations are a moving target, while the previous minor is not.

also re: #21 DrupalCI supports testing against any core branch thats open, as well as against the latest stable release. It does not always run against the latest core branch.

Another slight curve-ball the throw into the mix here is upstream deprecations when using a composer based workflow (such as drupal-composer/drupal-project which I understand is the recommendation). As well as affecting any project level tests, this seems to be the 'go to' method for running crontrib tests outside of drupal.org's CI (e.g. github or gitlab), where you create a drupal project and include the module, then runs it's tests.

With that workflow, the dependencies of Drupal are included via drupal/core's composer.json. This means that you don't run against the exact dependencies in core's composer.lock and as a result can hit deprecations in it's dependencies.

The example I've just hit is that symfony/psr-http-message-bridge yesterday released 1.2.0 which matches core's dependency of ^1.0 and so is installed, but deprecates Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory which is still used in core.

@andrewbelcher yeah core has a list of deprecations it ignores for those cases.

@Gábor Hojtsy is that something committed in core, or part of drupal.org's test infra? It would be quite handy to have some visibility of that.

@andrewbelcher See https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21Listen...

The key here is multiple branches, as deprecation testing by itself is already supported.

Reparenting.

Posted #3225792: Allow core deprecation testing to distinguish between major versions which is not quite the same, but related.