Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits

If you say this:

grep -rl 'use Drupal\\simpletest\\WebTestBase' ./ | xargs sed -i '' 's/use Drupal\\simpletest\\WebTestBase/use Drupal\\Tests\\BrowserTestBase as WebTestBase/g'

You end up with this patch.

Which will fail.

Apocalyptically. :-)

This time with https://www.drupal.org/files/issues/browsertest-convert.php_.txt

Already I noticed that there are some tests which use traits in the same namespace, which are missing after the move.

And the patch. :-)

This is awesome!

Some things I noticed looking at #2 (which is actually a pretty clever way to start out here):

1. fail: [Other] Line 121 of core/modules/filter/src/Tests/FilterFormatAccessTest.php:
   Drupal\filter\Tests\FilterFormatAccessTest::testFormatPermissions
   PHPUnit_Framework_Exception: Fatal error: Cannot use object of type Behat\Mink\Element\NodeElement as array in /var/www/html/core/modules/filter/src/Tests/FilterFormatAccessTest.php on line 153

   43 like that (I think) already in the tests that managed to report results; I wonder if a temporary BC shiv with ArrayAccess would be useful? No idea whether the object structure is remotely similar.

2. WTB::drupalGetTestFiles() has 45 usages. I bet that could live in a trait.

3. fail: [Other] Line 58 of core/modules/file/src/Tests/Views/RelationshipUserFileDataTest.php:
   Drupal\file\Tests\Views\RelationshipUserFileDataTest::testViewsHandlerRelationshipUserFileData
   PHPUnit_Framework_Exception: PHP Strict Standards:  Declaration of Drupal\file\Tests\Views\RelationshipUserFileDataTest::setUp() should be compatible with Drupal\views\Tests\ViewTestBase::setUp($import_test_views = true) in /var/www/html/core/modules/file/src/Tests/Views/RelationshipUserFileDataTest.php on line 17
   

   That's interesting.

Also noticed a couple things to discuss on #2770549: [plan] Discuss desired API and test suite following deprecation of SimpleTest.

1. AssertContentTrait::getRawContent() has 64 usages; might be another good candidate for a BC wrapper for BTB.
2. WTB::assertCacheContext() has 20 calls. That does not seem like it belongs on the main class. Looking at the code, it might be doable as a shared trait. We already have BC for drupalGetHeader().
3. drupalPost()/curlExec()/curlInitialize() look like an interesting chunk of things. (Also drupalPost() is a too-confusing name with drupalPostForm().)
4. Update tests are a whole can of worms; on a casual scan, they might merit their own issue in a hypothetical mass "convert all the things" plan.

5. fail: [Other] Line 67 of core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php:
   Drupal\field\Tests\EntityReference\EntityReferenceAdminTest::testFieldAdminHandler
   Argument 1 passed to Drupal\field\Tests\EntityReference\EntityReferenceAdminTest::getAllOptionsList() must be an instance of SimpleXMLElement, instance of Behat\Mink\Element\NodeElement given, called in /var/www/html/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php on line 508 and defined
   

   Similar to #9.1.

6. The xpath content assertions are a big chunk for a BC layer.

(Also drupalPost() is a too-confusing name with drupalPostForm().)

Only confusing if you're used to D7. ;-) https://www.drupal.org/node/2087433

Edit: moving my comments to the other issue.

@dawehner, also, I guess #15 was on the wrong issue so let's discuss on #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

#2782309: Refactor File and Image related image field creation logic into a new trait implements some of my feedback from #9.

We need to trigger the testbot for @dawehner's patch.

@xjm: opened #2784263: Move WTB::assertCacheContext() to AssertPageCacheContextsAndTagsTrait to consolidate assertCacheContext() with Simpletest.

Oh, exciting! Thanks for the painful and hard work dawehner. If we can make this green by just converting everything that passes right now I would RTBC it. Even more super cool would be to get this ready before 8.2 releases :)

Hm strange, even BrowserTestBaseTest is failing with this patch. Can you check if it passes for you locally with the 2 new test methods it has?

+++ b/browsertest-convert.php
@@ -0,0 +1,553 @@
+  'SimpleTestBrowserTest',
...
+  'SimpleTestBrowserTest',

Only a question : Why are we skipping the same test two times ?
Retested for 8.2.x, patch not applied, movig back to N/R

+1 on limiting to 8.3.x and then moving to 8.2.x if possible. 8.2.x changes would probably be non-disruptive, and would be allowed in beta since this is a testing improvement.

1. diff --git a/browsertest-convert.php b/browsertest-convert.php
   

   Should we keep this script in the repo? Maybe under core/scripts so contrib can use it? If we're going to do that, it'll need some more polish.

2. +++ b/core/modules/action/tests/src/Functional/ActionListTest.php
   @@ -1,15 +1,15 @@
   diff --git a/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
   
   diff --git a/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
   new file mode 100644
   
   new file mode 100644
   index 0000000..a6c9c32
   
   index 0000000..a6c9c32
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
   

   There are a bunch of these base classes in the patch that seem to come from nowhere. I talked with @dawehner in IRC about it and they're being copied for backwards compatibility, which is good.

Those new test bases should be almost similar copies from Simpletest, right? Why does the patch not show them as new copy/move? Can you roll the patch again with git --find-copies or -M25% or something? If the code is the same as in the Simpletest classes can we share it via traits?

browsertest-convert.php should not stay in the patch, but we can keep it until the patch is fully polished.

Added some more exceptions to the script:
* do not convert Views tests, Installer tests, Module tests etc.
* do not copy Traits because we just want to reuse them from phpunit (this will likely cause some fails because I forgot use statements)
* mark old test base classes as deprecated. That way Git can pick up the moved file in the diff and we just see the actual changes.

Opened #2798097: Move all kernel tests to their correct phpunit location.

Fixed use statement and conversion script.

Cool this is green now.

What do we do with this patch now? Make an announcement that we will commit this on for example Sept 28th to 8.3.x and 8.2.x? Or is it to late for 8.2?

If it is too late for 8.2 how do we proceed? Make up another random date after 8.2 is released? Or do we have to wait for the next code freeze phase in March 2017 for 8.3?

I propose that we do this before 8.2 is released to keep momentum, would be cool to get some core committer feedback.

So both https://www.drupal.org/pift-ci-job/463727 and https://www.drupal.org/pift-ci-job/463729 include something like:
Behat\Mink\Exception\ExpectationException: Link with label ||m`>&!s found.

Sounds like a classic random machine name vs. random string bug; is there any chance we're surfacing an issue like that?

Random test failures from #80 seems to be caused by #2808085: Pipe char in locators break Mink and Symfony element search. Occasionally the random string generator adds the pipe in output, then this text/label is used in Mink locators.

Postponing on #2808085: Pipe char in locators break Mink and Symfony element search.

Here is a patch from the phpunit initiative feature branch at https://github.com/klausi/drupal/tree/8.3.x-phpunit

Swapped out Mink for our patched version (#2808085: Pipe char in locators break Mink and Symfony element search) and applied #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests, which failed the testbot. So this is also likely going to fail.

Opened #2814035: Make $modules property protected on BrowserTestBase and KernelTestBase.

Opened #2814047: BrowserTestBase should implement AssertMailTrait for backwards compatibility.

Committed those 2 issues to the branch.

Aha, this is the same random test fail as experienced earlier in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests. Let's find out there what causes this.

Rolled in #2820442: SimpleTestBrowserTest has random fails when BrowserTestBaseTest gets larger and updated patches from other issues.

Ah, forgot to revert an earlier patch from #2814035: Make $modules property protected on BrowserTestBase and KernelTestBase, done now.

Yay, it's green! Queuing a bunch of test runs to make sure we have indeed eliminated the random test fails. Please also review all the issues I linked further above.

That is looking *very* green! Good stuff. Applied well for me, and I'm spot-checking a few of the small number tests I'm actually already familiar with.
What can we do to help next? (barring running hours of re-tests locally). I had a look, but can't see any action/help I can provide on the related issues I could see whilst this mega-change is pending here.

Patch rerolled, not other changes.

Unfortunately we saw 2 random test failures: a PHP segmentation fault on PHP 5.5 and Postgres and an update path test fail on SQLite. I don't understand yet if they relate at all to this conversion or not.

@dman: you can help in #2809161: Convert Javascript/AJAX testing to use JavascriptTestBase and #2809181: Provide forward compatibility layer for BrowserTestBase::xpath for example :)

Implemented the automatic addition of AssertMailTrait when the old Simpletest used drupalGetMails(). That means we don't need #2814047: BrowserTestBase should implement AssertMailTrait for backwards compatibility anymore.

Rerolled after #2808085: Pipe char in locators break Mink and Symfony element search, no other changes.

Now the only remaining dependency is #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests.

We fixed a random test fail in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests, merged that in.

The direction of this issue has changed slightly, we are now also adding traits to converted test classes because we don't want to make BrowserTestBase larger.

Merged in 8.3.x, added 2 tests to the conversion script that cannot be automatically converted right now.

#2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests is still pending, let's hope it gets committed soon.

And the last one got committed. Simple reroll, no other changes.

Once this is green we can remove the conversion script from the patch.

We should also announce a date now when we want to commit this large phpunit conversion chunk.

Yay, so here is the patch without the conversion script.

This should now be fully ready - please review!

Looks like a random fail in Drupal\page_cache\Tests\PageCacheTest, queuing all environments again just to make sure.

We have one Drupal\aggregator\Tests\FeedLanguageTest fail complaining that Drupal is already installed (race condition with parallel tests?) and one Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest fail on PHP 7.1.x-dev which is an experimental environment.

So this should be good to go, can somebody double check and RTBC this?

We have one Drupal\aggregator\Tests\FeedLanguageTest fail complaining that Drupal is already installed (race condition with parallel tests?) and one Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest fail on PHP 7.1.x-dev which is an experimental environment.

@klausi, let's exclude them from this conversion and open 2 follow-ups just to be sure we're not breaking anything.

Hm. FeedLanguageTest was not converted and AjaxCssTest is a JS test. Ignore my comment.

Thanks!

As mentioned in #2807237: PHPUnit initiative we will also announce a date when we commit this conversion to 8.3.x. I propose February 27th 2017, shorty before 8.3.0-rc1 is released. I will poke the core committers now to get a date fixed and announced.

catch also said a week before RC would be good, so that would match Feb 20th as dawehner proposed. That also works for me.

That timeline sounds good to me as well! How exciting.

Assuming we go forward with this plan, we can announce it after we release the alpha.

Do we want to enforce no more WTB in new patch policy after this is fixed?

Created #2847678: Deprecate WebTestBase and its required classes in favour of BrowserTestBase as a f/up.

OK, so let's aim for Feb 20th. And this will go into the 8.3.x branch.

Drafting an announcement text in the issue summary. What else should we mention in the announcement?

A datetime.module class snuck into a recent patch. This removes it (I just hand edited #106 and removed the hunks); will be part of #2780063: Convert web tests to browser tests for datetime and datetime_range modules.

Keep forgetting to tag issues.

One minor thing:

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
@@ -600,7 +599,7 @@ public function doTestChangedTimeAfterSaveWithoutChanges() {
     // Test only entities, which implement the EntityChangedInterface.
-    if ($entity instanceof EntityChangedInterface) {
+    if ($entity->getEntityType()->isSubclassOf('Drupal\Core\Entity\EntityChangedInterface')) {
       $changed_timestamp = $entity->getChangedTime();

this looks like an unrelated change, why is it necessary?

Sorry, that change has been introduced before in one of my earlier patches. Reverted.

I just learned that Feb. 20 is a US holiday and many businesses will be closed. I wonder if we should do this Feb. 21 instead?

Rerolled, interdiff is resolving the merge conflicts and rolling back what git wrongly merged.

@xjm: yes, Feb 21st also works!

fixing one more date.

I think that after this lands we need #2851541: [policy, no patch] No new WebTestBase tests in Drupal 8 after mass-conversion.

Only 8 days to go, will ping the release managers again on IRC to get the announcement out at https://groups.drupal.org/core before it is too late.

Only 4 days to go, I guess it does not make sense anymore to put out the announcement because nobody could react to it anyway over the weekend?

So I propose that we change the announcement to just inform people after the fact that stuff has been converted. I will change the proposed text after this has been committed on Tuesday :)

Just saw a response from xjm on IRC that I have now permission to publish announcements myself on https://groups.drupal.org/core . Published the announcement, better late then never! https://groups.drupal.org/node/516229

Thanks @klausi! Very sorry I was not able to follow up sooner; it got lost in the flood of things over the past week.

I am super excited!

BTW keeping this issue RTBC so it gets nightly retesting was a good choice; it helps us make sure that we are no longer introducing any random failures that are not accounted for by known issues. I can confirm everything on the issue so far is also present in HEAD because I get emails every time a core branch fails and I've seen them all before. :P

Or, more formally, #2829040: [meta] Known intermittent, random, and environment-specific test failures lists most of them.

Thanks a lot for watching out xjm! I really think we should immediately disable all random test failure prone test cases to prevent you getting the same fail emails every day.

I think it is 21st February in every timezone now. :D *fingers crossed*

@jibran Well, for the Americans, committing patches at 2/4/5a is probably not a great idea. ;)

Keeping an eye out now on the queued tests. FWIW, I also confirmed that the conversion does not conflict with our two RC-sensitive BC issues (or reverts thereof), #2751325: All serialized values are strings, should be integers/booleans when appropriate and #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing, since both use our modern testing APIs already.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Think there is a weird regression with the conversion, https://www.drupal.org/node/2745123#comment-11949935

Will post followup, but I am not 100% sure what is happening to make a real bug report yet, other than something potentially different in the way test teardown works that make the language manager unhappy.

This way merits an RN mention.

@mpdonadio, did you end up posting that followup? Sounds potentially important?

#148, no I think @daehner, @klausi, and @alexpott are of the opinion that this isn't really related. It has more of the other issue doing something that it shouldn't be.