Problem/Motivation

Follow-up from #3351236: Fix PHPStan L1 errors "Missing call to parent::setUp()/tearDown() method.".

In parent we left out fixing DrupalKernelTest::setUp(). That needs a change to KernelTestBase too, so it's worth discussing it here separately.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#19 3354063-19.patch9.06 KBSpokje
#17 3354063-17.patch9.07 KBGauravvvv
#14 3354063-14.patch9.18 KBlongwave
#11 3354063-11.patch9.3 KBlongwave
#2 3354063-2.patch2.36 KBmondrake

Issue fork drupal-3354063

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Active » Needs review
FileSize
3354063-2.patch2.36 KB

Here's a patch.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Marking this but there are 135 instances of Missing call to parent how are they being covered?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

#3 see parent issue

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs review

Hm, can DrupalKernelTest just override bootKernel() to do nothing?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Personally, I think this is more readable and gives more flexibility, than starting to override the base methods that setup the environment. Also, this is kind of a reference issue for further work - there are other similar issues (see #3351236: Fix PHPStan L1 errors "Missing call to parent::setUp()/tearDown() method.") but for BrowserTestBase.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -247,6 +247,16 @@ protected function setUp(): void {
+   * Extending tests may override this to tune their boot level to their
+   * specific need.

If this method is about the boot level, could we give it a name that reflects that?

I'm really not keen on Foo()/doFoo() pairs.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Would ::kernelSetUp() be a better option?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

bootKernel() is currently private presumably because we don't really want downstream users messing with the kernel setup. Except in this test, where we do! I personally think the simplest solution is to make bootKernel() protected instead and then just override it. If other kernel tests really want to mess with the boot process we shouldn't stop them; I would perhaps argue differently if this was in runtime code, but as it's tests-only I think we can and should be more flexible here.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Uhm then I think we should enlarge the scope of this issue to all remaining cases, and find a solution across them all.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Fix missing call to parent::setUp() in DrupalKernelTest » Fix missing call to parent::setUp() in remaining tests
FileSize
3354063-11.patch9.3 KB

Attached patch handles all remaining missing calls to parent::setUp(). I think there is further simplification that can be done in UpdatePathTestBase (zlib check, update URL generator, etc) but that can be deferred to a followup.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

Looks good and fixes the PHPStan reported errors. I would have preferred my own way… but that’s not matter to prevent rtbc. Thanks!

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Reviewed & tested by the community » Needs work

needs rebase

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3354063-14.patch9.18 KB
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Rebase seems good.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Needs followup

Needs a reroll.

longwave suggested a followup in #11. Has that been made?

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3354063-17.patch9.07 KB

Re-rolled patch #14. Removed re-rolled tag. please review

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Issue tags: -Needs followup
FileSize
3354063-19.patch9.06 KB

Rerolled, opened follow-up #3360167: Simplification in UpdatePathTestBase (thanks @quietone)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3354063-19.patch, failed testing. View results

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs work » Needs review
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Looks like a random fail: https://www.drupal.org/project/drupal/issues/2829040#comment-15050937

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Yea a number of tickets are getting this random

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3354063-19.patch, failed testing. View results

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Rerolled 3354063-19_0.patch on 11.x-dev in MR

Spokje opened merge request !4055

  • longwave committed 7e4ecad5 on 11.x 
    Issue #3354063 by Spokje, longwave, mondrake, Gauravvvv, joachim: Fix...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7e4ecad537 to 11.x (10.2.x). Thanks!

Spokje closed merge request !4055

Status: Fixed » Closed (fixed)

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