Problem/Motivation

\Drupal\Tests\ban\Functional\IpAddressBlockingTest inefficiently logs in a user and submits a form several times to test validation that could be done in a unit test. There is some database interaction, so we can split that part to a kernel test.

Before: ~16 seconds
After: ~1 second

Steps to reproduce

Proposed resolution

  • Test BanIpManagerInterface::banIp in a kernel test (BanIpTest::testBanIp)
  • Test IP validation in a unit test (BanAdminTest::testIpValidation)
  • Test form submission calls BanIpManagerInterface::banIp in a unit test (BanAdminTest::testFormSubmission)
  • Test form build adds default ip from route param in a unit test (BanAdminTest::testRouteParameter)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3468435

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

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !9224

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
mstrelan’s picture

mstrelan commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Title: Convert IpAddressBlockingTest to a Unit and Kernel test » Convert IpAddressBlockingTest to a Unit and Kernel test and improve
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Nice find!

Tweaked the title slightly since it appears the the tests are being improved upon, evident by your comment in the MR too.

From what I can tell no coverage has been lost, as mentioned shaves a few seconds off, and even got to improve the tests it appears.

  • catch committed 28ac4713 on 10.3.x 
    Issue #3468435 by mstrelan: Convert IpAddressBlockingTest to a Unit and...

  • catch committed 6cf07f9a on 10.4.x 
    Issue #3468435 by mstrelan: Convert IpAddressBlockingTest to a Unit and...

  • catch committed 292aee30 on 11.x 
    Issue #3468435 by mstrelan: Convert IpAddressBlockingTest to a Unit and...

  • catch committed 978aeeec on 11.0.x 
    Issue #3468435 by mstrelan: Convert IpAddressBlockingTest to a Unit and...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Yes this looks great.

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch closed merge request !9224

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Looks like core/modules/ban/tests/src/Unit/BanAdminTest.php is failing now?

------ ----------------------------------------------------------------------
Line core/modules/ban/tests/src/Unit/BanAdminTest.php
------ ----------------------------------------------------------------------
36 Class Drupal\ban\Form\BanAdmin constructor invoked with 1 parameter,
2 required.
74 Class Drupal\ban\Form\BanAdmin constructor invoked with 1 parameter,
2 required.
95 Class Drupal\ban\Form\BanAdmin constructor invoked with 1 parameter,
2 required.
------ ----------------------------------------------------------------------

See #3427545: Allow bulk unbanning / removal and clearing ip addresses (Flush all). Should that be fixed here (reopen) or separate issue?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Update: Sorry, I was wrong, all fine here! The other issue caused the test to fail, a comment was misleading.

All good!

Status: Fixed » Closed (fixed)

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