Problem/Motivation

In #2304461: KernelTestBaseTNG™ we planned the removal of the deprecated KernelTestBase class, but we decided to only do it in Drupal 9.

Proposed resolution

Update the deprecation message.

Remaining tasks

Change the deprecation message to 9.x, see \Drupal\simpletest\KernelTestBase

User interface changes

None.

API changes

none

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
73.67 KB

Here is a start, not completely done yet. Searching for usage with ag simpletest\\\\KernelTestBase.

Status: Needs review » Needs work

The last submitted patch, 2: kernelbase-remove-2734663-2.patch, failed testing.

jibran’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
86.12 KB
12.45 KB

It's the final countdown for the old test base! Let's continue here.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Let's do it!

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

I suppose the question is now when are we going to commit this exactly and how much time to we give contrib to catch up?

Looking at my local checkout, I see 5 remaining tests in 5 different modules (address, embed, libraries, payment, plugin). Most of those where I have commit access I've already converted.

@jibran: Even if we decide to not wait any longer, there's still a @todo ? in the patch, I think that's simply a different method on the new class.

jibran’s picture

dawehner’s picture

To be honest my plan was to convert just all the remaining tests in its own issue and then suggest to commit the removal patch quite late in the 8.2.x cycle.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
86.18 KB
618 bytes

Fixed the remaining "@see ?".

I think we should commit this as soon as possible to give contrib modules an early warning to update their kernel tests before 8.2.0 is released. I'm not familiar with the testbot for contrib on drupal.org, but if I look at the automated testing tab for the libraries module Jenkins tells me it is running against 8.1.x core https://dispatcher.drupalci.org/job/default/54415/ . So when we commit this to 8.2.x contrib tests will not be affected at all? Even more reason that we can commit this right now?

After the testbot agrees this should go back to "needs work" for a change record.

dawehner’s picture

Berdir’s picture

@klausi: that test run is from 20.12.2015 :) Testbot defaults to 8.2.x already since before 8.1.0 came out.

klausi’s picture

ok, but since we are only breaking tests and not the functionality of contrib modules I still think it is ok to go ahead with this. What would be the benefits of doing it later? At some point we will break contrib?

Berdir’s picture

Yes, we will eventually break it.

KernelTestBase is the only thing that is deprecated for removal in 8.2.x, we don't do that with anything else, so I'm not sure how aware contrib maintainers are of this.

Maybe we could do it like we've done with other changes in the past, a public announcement that it will get removed on day X and then do it then.

Up for the core maintainers to decide this, I'm also OK if we go ahead, just wanted to make sure that the implications are clear.

dawehner’s picture

I think this would be a fair tag to add at this time.

Sam152’s picture

Re: #7, out of 71 modules I have checked out locally, the only additional one I can see with tests left to convert that you haven't already mentioned is config_devel.

jibran’s picture

jibran’s picture

Issue summary: View changes
Status: Needs review » Postponed
FileSize
79.33 KB

This is now postpone on #2734423: Convert the remaining old kernel tests over because I moved the tests conversion to #2734423: Convert the remaining old kernel tests over. This is just removing the KernelTestBase and abstract classes. We can re-upload the attached patch once #2734423: Convert the remaining old kernel tests over is committed.

jibran’s picture

jibran’s picture

Status: Postponed » Needs review
FileSize
79.33 KB

Re-uploading #18.

catch’s picture

Yeah I think we should announce this in advance, commit the patch afterwards.

While it will only break tests (and converting can be done against 8.1.x), contrib tests still run against 8.2.x by default, so if you're trying not to maintain a project at the absolute minimum, having to port due to an 8.2.x change months before it will get released. Which makes me think committing this shortly before the beta could be good too.

jibran’s picture

Let's create a g.d.o post for announcement.

jibran’s picture

Issue tags: +beta target

How about this as g.d.o/core post?

In the effort of removing simpletest module in Drupal 9 and completely replace it by PHPUnit. Now, all the core Kernel Tests are executed using PHPUnit test runner and core is going focus on converting all the simpletest web tests to browser tests executed using PHPUnit.

Before the first beta release of 8.2.x the old \Drupal\simpletest\KernelTestBase will be removed in #2734663: Update deprecation message for old KernelTestBase in simpletest. It was deprecated in 8.0.x. All the core kernel tests are using the new base class \Drupal\KernelTests\KernelTestBase now. Most of the contrib modules have also converted their kernel tests but if your contrib or custom module is still using old \Drupal\simpletest\KernelTestBase please have a look at https://www.drupal.org/node/2489956.

xjm’s picture

Thanks for the draft @jibran! I'd remove any mentions of Drupal 9 or removing SimpleTest; both will unnecessarily alarm people. :) So it would go something like:

As part of our ongoing efforts to modernize Drupal's automated testing, all the core Kernel Tests are executed using PHPUnit test runner.

Before the first beta release of 8.2.x the old \Drupal\simpletest\KernelTestBase will be removed in #2734663: Remove deprecated KernelTestBase from simpletest. It was deprecated in 8.0.x. All the core kernel tests are using the new base class \Drupal\KernelTests\KernelTestBase now. Most of the contrib modules have also converted their kernel tests, but if your contrib or custom module is still using old \Drupal\simpletest\KernelTestBase please have a look at the change record for instructions on updating your tests.

The next step for this initiative will be to convert SimpleTest web tests to browser tests executed using PHPUnit.

I'm kind of surprised we deprecated this for removal in a minor TBH; our BC policy says:

The contents of automated tests provided by core modules are never considered part of the API. While the testing framework itself may be treated as an API individual test classes are always internal.

I would definitely consider KernelTestBase part of the "testing framework". I can see the case for removing it to reduce unmaintained/untested code and for DX, since it is not something that should be used in production anyway, but it does feel like a public API BC break. The g.d.o/core post is a good mitigating step if we go ahead with this.

Mixologic’s picture

As a testbot maintainer, I sure would love a heads up when this is planned to be committed. It'll be a lot easier to field all the issues/questions from contrib maintainers who suddenly have broken tests.

As an aside, is there something that drupalci could be doing to report this deprecation? I.e it sure would be nice to see, in your test results, that you need to update your tests, that they will cease functioning as soon as 8.2.0 is released if you do not update them.

dawehner’s picture

I'm kind of surprised we deprecated this for removal in a minor TBH; our BC policy says:

Well, just to be clear, when we added this feature back over a year ago, we added this deprecation notice to the old KernelTest. No matter what people say, it was in there since a long long time. Technically it is not a surprise for people.

On top of that its pretty easy to convert tests over. Even the hard ones in core have been fairly reasonable.

I can see the case for removing it to reduce unmaintained/untested code and for DX, since it is not something that should be used in production anyway, but it does feel like a public API BC break.

Technically of course its an API BC break, but as the docs say, "may be". It basically IMHO comes down to provide a trade off between the benefits: not having to maintain this class/gaining time for people, as they profit from better tools vs. the issues of broken tests.

Maybe we could announce it now, and maybe remove it in 8.3 or so? A public announcement IMHO helps people better than some @deprecate in their code ...

As a testbot maintainer, I sure would love a heads up when this is planned to be committed. It'll be a lot easier to field all the issues/questions from contrib maintainers who suddenly have broken tests.

Well, it would be nice if the testbot could somehow have the notion of warnings on tests. This could be helpful in multiple places:

  • Core could actually throw E_USER_NOTICE errors while not failing tests, but rather let the testbot let people know about it. Of course this would require work in the testing system itself
  • We could warn people about other deprecated stuff
jibran’s picture

it does feel like a public API BC break

No it is not. We converted all the KernelTestBase for contrib as well and if someone is using KernelTestBase in d8 for personal or client site it means they are smart enough to figure out the conversion using the change record. Please do not over think this. Do you still consider it an public API BC break if no one is using this at all?

This is frustrating we are less then week away from beta and we still haven't posted an announcement on which release manager(#21) signed off 1.5 months ago.

dawehner’s picture

Conceptually its still a bit weird, we made the decision to deprecate as of 8.2 when it got committed and catch agreed on #21 to have the post. Maybe its a bit frustration of myself, but its hard to see the point of decisions, when they are actually not.

xjm’s picture

Issue tags: -beta target

Only committers should add the beta target tag. Thanks!

jibran’s picture

I only added the tag because it made sense to have this change before first beta release. I didn't know there is any rule for adding the tag.

dawehner’s picture

It is a bit bad that the communication we have to users since a year, KTB will be removed, seemed to not be actually true.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

I haven't seen any actual objections here, just concerns that I think can be addressed (eg, a core announcement + change notice).

The code has been in place for a year now stating that this class will be removed before 8.2.x, so anybody using an IDE that supports @deprecated tags has seen this notice many times (it's also called out quite visibly on the API page).

+1 for RTBC here.

dawehner’s picture

Let's target 8.3.x

@xjm
Can we maybe agree on that?

dawehner’s picture

Status: Needs review » Needs work

We (xjm, alexpott, klausi, jibran, dawehner, people I'm missing) agreed to not remove the functionality, but update the deprecation message to Drupal 9.

dawehner’s picture

Issue summary: View changes
Issue tags: +Novice

Let's add a novice task for that.

klausi’s picture

Title: Remove deprecated KernelTestBase from simpletest » Update deprecation message for old KernelTestBase in simpletest
Issue summary: View changes

ok, so let's only change the deprecation message.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
616 bytes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@naveenvalecha
Thank you!

klausi’s picture

Issue tags: +Quickfix

RTBC+1.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Quickfix +rc eligible

Committed and pushed 49076af to 8.3.x and 835b98e to 8.2.x. Thanks!

  • alexpott committed 49076af on 8.3.x
    Issue #2734663 by klausi, jibran, dawehner, naveenvalecha: Update...

  • alexpott committed 835b98e on 8.2.x
    Issue #2734663 by klausi, jibran, dawehner, naveenvalecha: Update...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.