Problem/Motivation

\Drupal\Tests\system\Kernel\Functional\HtmlToTextTest Makes no HTTP requests but is a functional test.

Steps to reproduce

Proposed resolution

Convert \Drupal\Tests\system\Kernel\Functional\HtmlToTextTest into a Kernel test.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3414914

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

Spokje created an issue. See original summary.

Spokje opened merge request !6191

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Needs work
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community
mstrelan’s picture

mstrelan commented
Status: Reviewed & tested by the community » Needs work

Couldn't this be a unit test? Think we just need to ensure the global $base_url and $base_path variables are set.

mstrelan’s picture

mstrelan commented
Title: Convert HtmlToTextTest into a Kernel test » Convert HtmlToTextTest into a Unit test
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented

Thanks @kim.pepper for being lenient, I know technically you're absolutely right and the type hinting is out of scope.
But I feel BC and the (correct) tight scoping is slowing drupal down sometimes and hope we can be a bit more "liberal" sometimes with tests only.

@mstrelan: Very nice Sir!
Can't RTBC since I worked on it, but looks good to go to me.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Unit test looks even better!

  • catch committed 4a87e7e8 on 10.2.x 
    Issue #3414914 by mstrelan, Spokje, kim.pepper: Convert HtmlToTextTest...

  • catch committed 85e2310a on 11.x 
    Issue #3414914 by mstrelan, Spokje, kim.pepper: Convert HtmlToTextTest...
catch’s picture

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

fwiw given it's only test changes, I think it's OK to sneak some cleanup in at the same time, given the code changes are pretty minimal. If it was runtime code, more than one test at a time (which I don't think we should do for these because most have one or two or more changes beyond a rename) or made it harder to review, I would probably push back though.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

spokje’s picture

spokje
Primary language Dutch
commented

fwiw given it's only test changes, I think it's OK to sneak some cleanup in at the same time, given the code changes are pretty minimal

Thanks @catch, I was hoping this would be the case.

more than one test at a time (which I don't think we should do for these because most have one or two or more changes beyond a rename) or made it harder to review,

No worries, I'll do one at a time, we all know I'm just in it for the core credits... ;)

Spokje closed merge request !6191

Status: Fixed » Closed (fixed)

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