Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2734663-38.patch | 616 bytes | naveenvalecha |
#20 | remove_deprecated-2734663-20.patch | 79.33 KB | jibran |
Comments
Comment #2
klausiHere is a start, not completely done yet. Searching for usage with
ag simpletest\\\\KernelTestBase
.Comment #4
jibranDuplicate of #2734423: Convert the remaining old kernel tests over.
Comment #5
dawehnerIt's the final countdown for the old test base! Let's continue here.
Comment #6
jibranLet's do it!
Comment #7
BerdirI 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.
Comment #8
jibranCreated PR for embed https://github.com/drupal-media/embed/pull/26
Comment #9
dawehnerTo 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.
Comment #10
klausiFixed 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.
Comment #11
dawehnerhttps://www.drupal.org/node/2734937
Comment #12
Berdir@klausi: that test run is from 20.12.2015 :) Testbot defaults to 8.2.x already since before 8.1.0 came out.
Comment #13
klausiok, 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?
Comment #14
BerdirYes, 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.
Comment #15
dawehnerI think this would be a fair tag to add at this time.
Comment #16
Sam152 CreditAttribution: Sam152 commentedRe: #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.
Comment #17
jibran@Sam152 Please see #2734835: Convert Kernel tests to KernelTestBaseNG.
Comment #18
jibranThis 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.
Comment #19
jibranComment #20
jibranRe-uploading #18.
Comment #21
catchYeah 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.
Comment #22
jibranLet's create a g.d.o post for announcement.
Comment #23
jibranHow 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.Comment #24
xjmThanks 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:
I'm kind of surprised we deprecated this for removal in a minor TBH; our BC policy says:
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.
Comment #25
MixologicAs 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.
Comment #26
dawehnerWell, 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.
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 ...Well, it would be nice if the testbot could somehow have the notion of warnings on tests. This could be helpful in multiple places:
Comment #27
jibranNo 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.
Comment #28
dawehnerConceptually 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.
Comment #29
xjmOnly committers should add the beta target tag. Thanks!
Comment #30
jibranI 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.
Comment #31
dawehnerIt is a bit bad that the communication we have to users since a year, KTB will be removed, seemed to not be actually true.
Comment #33
jhedstromI 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.
Comment #34
dawehnerLet's target 8.3.x
@xjm
Can we maybe agree on that?
Comment #35
dawehnerWe (xjm, alexpott, klausi, jibran, dawehner, people I'm missing) agreed to not remove the functionality, but update the deprecation message to Drupal 9.
Comment #36
dawehnerLet's add a novice task for that.
Comment #37
klausiok, so let's only change the deprecation message.
Comment #38
naveenvalechaComment #39
dawehner@naveenvalecha
Thank you!
Comment #40
klausiRTBC+1.
Comment #41
alexpottCommitted and pushed 49076af to 8.3.x and 835b98e to 8.2.x. Thanks!