On or about 2017/02/21, #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits is going to be committed to 8.3.x This will result in many of our WebTestBase (WTB) tests being converted into BrowserTestBase (BTB) tests.

There are some good reasons we don't want to #2847678: Deprecate WebTestBase in favor of BrowserTestBase yet, mainly because not all of our tests have been converted yet and there are some issues surrounding having total replacement for WTB. However, there is the possibility that we could introduce new WTB into 8.3.x or 8.4.x, and just have to convert more things down the road.

I am proposing that after 2770921 lands,

  • We no longer accept patches that introduce entirely new WTB test classes, except at subsystem, release, or framework manager discretion
  • If a WTB already exists, and additional coverage is appropriate in that class, then add in new tests methods or assertions as needed

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

I went through the related issues and didn't see this formally discussed. Even if this has been discussed offline or in a comment on another issue, it would be nice to have an official issue to point to to say "We need to convert this" when doing patch reviews.

klausi’s picture

Issue summary: View changes

The big conversion is going to happen in the 8.3.x branch.

Otherwise I'm unsure if we should have such a policy right now. I don't want to discourage contributors with yet another core gate of using PHPUnit to get stuff done. We could define a goal of conversion - for example once 80% of web test + browser test classes are PHPUnit we reject patches with new Simpletests.

Number of browser tests in core

find . -path *tests*Functional* -type f | wc -l

in 8.4.x right now: 144
After conversion patch: 394

Number of Simpletests in core

find . -path */src/Tests/* -type f | wc -l

in 8.4.x right now: 1014
After conversion patch: 782

Converted in 8.4.x: (144 / (144 + 1014)) * 100 = 12%
Converted after conversion patch: (394 / (394 + 782)) * 100 = 33.5%

Those numbers are not completely accurate because that also counts traits and some other minor classes. It is also a bit biased to Simpletest because we cannot remove Simpletest base classes, we can only deprecate them.

So after the big conversion patch we will have one third of core converted which IMO is a bit low to make demands of contributors yet.

dawehner’s picture

One thing we could say is: If you write a new test class, use BTB. If there is a well fitting existing WebTestCase, work on that one.

xjm’s picture

Title: [policy,nopatch] No new WebTestBase tests in Drupal 8 after mass-conversion » [policy, no patch] No new WebTestBase tests in Drupal 8 after mass-conversion
Status: Active » Reviewed & tested by the community
Issue tags: +Needs framework manager review, +Needs release manager review, +Needs subsystem maintainer review

I agree with @dawehner's proposal here. If @klausi agrees then we have consensus from the subsystem maintainers and can mark it RTBC. :) Also pinging the relevant committers.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Ehm I may have gotten a little over-eager there.

mpdonadio’s picture

Issue summary: View changes

#4 was essentially what I was getting at. Clarified IS.

Thought about adding something along the lines of "use UnitTestBase or KernelTestBase instead of a functional test if you don't need to simulate a browser", but decided against it.

Feel free to edit.

alexpott’s picture

I think #4 sounds totally sane and the clarified IS.

Re #7 I'm not sure - it can be tricky to decide and I'm not sure that "if you don't need to simulate a browser" caveat actually helps.

klausi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

Yep, fine with me also.

cilefen’s picture

I like it.

catch’s picture

+1

xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs framework manager review, -Needs release manager review

Okay, that sounds like a quorum!

I updated the core gates policy for this:
https://www.drupal.org/node/1203498/revisions/view/10353348/10353373

I also took the opportunity to update the policy for JSTB as the framework and release managers agreed awhile back.

jibran’s picture

I think after this policy we should create a follow-up to deprecate \Drupal\simpletest\WebTestBase? Or we'd do that like KTB after all the conversions?

klausi’s picture

Status: Fixed » Closed (fixed)

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