Convert first part of web tests of views_ui

1. +++ b/core/modules/views_ui/tests/src/Functional/CachedDataUITest.php
   @@ -58,6 +58,7 @@ public function testCacheData() {
   +    file_put_contents('/tmp/foo.html', $this->getSession()->getPage()->getHtml());
   

   debug statement that should be removed?

2. +++ b/core/modules/views_ui/tests/src/Functional/DefaultViewsTest.php
   @@ -13,6 +14,8 @@
    class DefaultViewsTest extends UITestBase {
    
   +  use BlockCreationTrait;
   +
   

   we use that block creation trait on so many browser tests, i think it should be on BrowserTestBase.

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -181,12 +196,98 @@ protected function assertTitle($expected_title) {
   +   *   Use $this->assertSession()->LinkByHref() instead.
   

   should be ->linkByHref()

4. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -181,12 +196,98 @@ protected function assertTitle($expected_title) {
   +   *   Use $this->assertSession()->LinkByHref() instead.
   

   same here

5. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -899,6 +919,11 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   +      // Provide support for 1, 0 for checkboxes instead of TRUE and FALSE.
   +      if (strpos($name, 'name[') === 0) {
   +        $value = (bool) $value;
   +      }
   

   should we really do this? Maybe we should fix all invocations to use proper TRUE/FALSE values? We should have at least @todo here to remove this behavior once all invocations are fixed.

Changes to webassert got committed, reroll without those changes.

+++ a/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -795,11 +795,6 @@
-      // Provide support for 1, 0 for checkboxes instead of TRUE and FALSE.
-      if (strpos($name, 'name[') === 0) {
-        $value = (bool) $value;
-      }
-

Oops this needed to stay in

Do you want to move this into a dedicate issue with some explicit test coverage?

Just looking at the code now and it turn out that this was already added in #2750941: Additional BC assertions from WebTestBase to BrowserTestBase, so Oops, it needs to stay out!

So #18 is the one to build on.

Current HEAD has:

      // Provide support for the values '1' and '0' for checkboxes instead of
      // TRUE and FALSE.
      // @todo Get rid of supporting 1/0 by converting all tests cases using
      // this to boolean values.
      $field_type = $field->getAttribute('type');
      if ($field_type === 'checkbox') {
        $value = (bool) $value;
      }

Giving this a re-roll.

First thing was getting errors over \Drupal\Tests\views_ui\Functional\UITestBase::drupalGet not matching parent.

This won't fix everything, but should get us closer.

This reads a bit dangerous!

I removed that line because it's tested twice. I might have had a borked merge, patch needed re-roll.

Picking this back up at DrupalCon Dublin.

@dawehner, shoot :/ I forgot I had even attempted to pick this up. Drupal Commerce beta ate my time and I don't think I made headway. I think we need some of the other initiative issues to land, like #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests as well to make this simpler.

Unassigning for now.

Re-roll.

Work in progress patch, some namespace and constructor fixes to make some more tests working.

Reroll for array() to [] conversion.

@michielnugter bit of a jump in patch file size

Was trying to build upon #36 and #38 , but none of the patch applies properly.

@lendude: I forgot the -M tag..

@gaurav.kapoor: I'm planning to work on this issue this week and next week during the Sevilla Dev Days. If you prefer to work on this issue instead, ping me any time on irc or post a message here to let me know so I can work on other issues..

@michielnugter : all yours.

1. +++ b/core/modules/views_ui/tests/src/Functional/CustomBooleanTest.php
   @@ -173,8 +173,7 @@ public function testCustomOptionTemplate() {
   -      $this->setRawContent($output);
   -      $this->assertText('llama', 'Loaded the correct views-view-field.html.twig template');
   +      $this->assertTrue(strpos($output, 'llama') !== FALSE);
   

   let's use $this->assertContains() instead the strpos().

2. +++ b/core/modules/views_ui/tests/src/Functional/DefaultViewsTest.php
   @@ -42,7 +41,7 @@ public function testDefaultViews() {
   -    $this->clickViewsOperationLink(t('Enable'), '/glossary/');
   +    $this->clickViewsOperationLink('Enable', '/glossary/');
   

   unrelated change. This has nothing to do with the conversion, so we need to keep the wrong t() call in place for now.

3. +++ b/core/modules/views_ui/tests/src/Functional/DisplayFeedTest.php
   @@ -57,10 +57,9 @@ protected function checkFeedViewUi($view_name) {
   +        $options[] = (string) $input_node->getAttribute('value');
   

   The (string) cast can be removed, getAttribute() will always return a string.

4. +++ b/core/modules/views_ui/tests/src/Functional/DisplayFeedTest.php
   @@ -73,12 +72,12 @@ protected function checkFeedViewUi($view_name) {
   -    $this->assertFieldByXpath('//*[@id="views-feed-1-displays"]', '<em>Page</em>');
   +    $this->assertSession()->elementExists('xpath', '//*[@id="views-feed-1-displays"]');
   

   this change removes the check for the contents, but we should keep it somehow.

5. +++ b/core/modules/views_ui/tests/src/Functional/DisplayFeedTest.php
   @@ -73,12 +72,12 @@ protected function checkFeedViewUi($view_name) {
   -    $this->assertFieldByXpath('//*[@id="views-feed-1-displays"]', 'Multiple displays');
   +    $this->assertSession()->elementExists('xpath', '//*[@id="views-feed-1-displays"]');
   

   same here.

6. +++ b/core/modules/views_ui/tests/src/Functional/DisplayTest.php
   @@ -285,15 +285,15 @@ public function testActionLinks() {
   -    $this->assertFieldByXpath('//input[@type="submit"]', 'Duplicate ' . $display_title);
   -    $this->assertFieldByXpath('//input[@type="submit"]', 'Delete ' . $display_title);
   -    $this->assertFieldByXpath('//input[@type="submit"]', 'Disable ' . $display_title);
   -    $this->assertNoFieldByXpath('//input[@type="submit"]', 'Enable ' . $display_title);
   +    $this->assertSession()->elementExists('xpath', '//input[@type="submit"]');
   +    $this->assertSession()->elementExists('xpath', '//input[@type="submit"]');
   +    $this->assertSession()->elementExists('xpath', '//input[@type="submit"]');
   +    $this->assertSession()->elementNotExists('xpath', '//input[@type="submit"]');
   

   this conversion should not be necessary. The assertFieldByXpath method exists now and should work, right?

7. +++ b/core/modules/views_ui/tests/src/Functional/DisplayTest.php
   @@ -285,15 +285,15 @@ public function testActionLinks() {
   -    $this->assertFieldByXpath('//input[@type="submit"]', 'Enable ' . $display_title);
   -    $this->assertNoFieldByXpath('//input[@type="submit"]', 'Disable ' . $display_title);
   +    $this->assertSession()->elementExists('xpath', '//input[@type="submit"]');
   +    $this->assertSession()->elementNotExists('xpath', '//input[@type="submit"]');
   

   same here.

8. +++ b/core/modules/views_ui/tests/src/Functional/ExposedFormUITest.php
   @@ -32,7 +32,7 @@ class ExposedFormUITest extends UITestBase {
   -  protected function setUp() {
   +  protected function setUp($import_test_views = TRUE) {
   

   why do we change this everywhere?

9. +++ b/core/modules/views_ui/tests/src/Functional/FieldUITest.php
   @@ -39,27 +39,27 @@ public function testFieldUI() {
   -    $this->assertNoLinkByHref($edit_groupby_url, 0, 'No aggregation link found.');
   +    $this->assertNoLinkByHref($edit_groupby_url);
   

   I know our assertNoLinkByHref() method ignores the other parameters, but we should change as little as possible in this conversion patch.

10. +++ b/core/modules/views_ui/tests/src/Functional/FieldUITest.php
    @@ -67,12 +67,13 @@ public function testFieldUI() {
    -    $this->assertLinkByHref($edit_groupby_url, 0, 'Aggregation link found.');
    +    $this->assertLinkByHref($edit_groupby_url);
    

    same here

Progess! Because the list is quite long I decided to to categorize the tests first. What's working with minimal effort and what's not.

Non-working tests
AnalyzeTest.php
CachedDataUITest.php
CustomBooleanTest.php
DisplayAttachmentTest.php
DisplayCRUDTest.php
DisplayPathTest.php
DisplayTest.php
ExposedFormUITest.php
FilterBooleanWebTest.php
FilterNumericWebTest.php
HandlerTest.php
OverrideDisplaysTest.php
PreviewTest.php
RowUITest.php
SettingsTest.php
TaxonomyIndexTidUiTest.php
TokenizeAreaUITest.php
ViewsUITourTest.php
ViewTestBase.php
WizardTest.php
XssTest.php

Working tests
AccessRoleUITest.php
AreaEntityUITest.php
ArgumentValidatorTest.php
ConfigTranslationViewListUiTest.php
ContentTranslationViewsUITest.php
DefaultViewsTest.php
DisplayExtenderUITest.php
DisplayFeedTest.php
DuplicateTest.php
FieldUITest.php
FilterUITest.php
GroupByTest.php
NewViewConfigSchemaTest.php
QueryTest.php
RearrangeFieldsTest.php
RedirectTest.php
ReportFieldsTest.php
ReportTest.php
StorageTest.php
StyleTableTest.php
StyleUITest.php
TranslatedViewTest.php
UITestBase.php
UnsavedPreviewTest.php
ViewEditTest.php
ViewsListTest.php

Gone work some more on this. Will have to postpone on the Tour functional tests and views functional tests, view UI tests are using these bases.

--- a/core/modules/taxonomy/src/Tests/Views/TaxonomyParentUITest.php
+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyParentUITest.php
@@ -3,7 +3,7 @@
 namespace Drupal\taxonomy\Tests\Views;
 
 use Drupal\views\Tests\ViewTestData;
-use Drupal\views_ui\Tests\UITestBase;
+use Drupal\Tests\views_ui\Functional\UITestBase;
 
 /**
  * Tests views taxonomy parent plugin UI.

I added this missing change.

--- a/core/modules/views_ui/tests/src/Functional/OverrideDisplaysTest.php
+++ b/core/modules/views_ui/tests/src/Functional/OverrideDisplaysTest.php
@@ -12,7 +12,7 @@ class OverrideDisplaysTest extends UITestBase {
   protected function setUp() {
     parent::setUp();
 
-    $this->drupalPlaceBlock('page_title_block');
+    $this->placeBlock('page_title_block');
   }
 
   /**

I reverted this change.

I applied all suggestions from #49.

Tests are still failing on /core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidUiTest.php, line 96. I could not find a substitute for method attributes() in class \Behat\Mink\Element\NodeElement.

It seems we sent the patches at the same time.
The error you are getting at TaxonomyIndexTidUiTest.php will be fixed with the first snippet in my comment at #51

@Mac_Weber

I'll merge the patches later this morning and continue working on it at the Sevilla Dev Days. I have @lendude sitting next to me so any question I'll have I can have a quick answer to :)

Merged changes and got another working.

Non-working tests
CachedDataUITest.php
CustomBooleanTest.php
DisplayAttachmentTest.php
DisplayCRUDTest.php
DisplayPathTest.php
DisplayTest.php
ExposedFormUITest.php
FilterBooleanWebTest.php
FilterNumericWebTest.php
HandlerTest.php
OverrideDisplaysTest.php
PreviewTest.php
RowUITest.php
SettingsTest.php
TaxonomyIndexTidUiTest.php
TokenizeAreaUITest.php
ViewsUITourTest.php
ViewTestBase.php
WizardTest.php
XssTest.php

Working tests
AnalyzeTest.php
AccessRoleUITest.php
AreaEntityUITest.php
ArgumentValidatorTest.php
ConfigTranslationViewListUiTest.php
ContentTranslationViewsUITest.php
DefaultViewsTest.php
DisplayExtenderUITest.php
DisplayFeedTest.php
DuplicateTest.php
FieldUITest.php
FilterUITest.php
GroupByTest.php
NewViewConfigSchemaTest.php
QueryTest.php
RearrangeFieldsTest.php
RedirectTest.php
ReportFieldsTest.php
ReportTest.php
StorageTest.php
StyleTableTest.php
StyleUITest.php
TranslatedViewTest.php
UITestBase.php
UnsavedPreviewTest.php
ViewEditTest.php
ViewsListTest.php

Postponed on #2862947: Incorrect field assertions in AssertLegacyTrait.

As discussed offline with @klausi the issue will not be postponed on the Tour test. It will be removed from the scope of this issue and moved into the scope of the Tour conversion issue (#2767275: Convert web tests to browser tests for tour module).

Update:

Down to the following 'needs work'
DisplayTest.php
ExposedFormUITest.php
FilterNumericWebTest.php
PreviewTest.php
RowUITest.php
WizardTest.php
XssTest.php

Alright, after a lot of work I have all but a few that I moved out of scope passing.

WizardTest: #2863267: Convert web tests of views
Base class is in the views module and @lendude is working on that.

TourTest: #2767275: Convert web tests to browser tests for tour module
Base class is worked on in TourTest. This test should either be converted in that issue or split into a separate issue and postponed on the tour issue.

RowUITest: #2809553: Convert AJAX part of \Drupal\views_ui\Tests\RowUITest to BrowserTestBase
This test requires a JavascriptTestBase conversion.

PreviewTest:#2863563: Convert PreviewTest WebTestBase to BrowserTestBase and JavascriptTestBase
This test requires a partial JavascriptTestBase conversion and requires a new ViewTestBase class for JavascriptTestBase. Split of into a separate issue so we can postpone it on the JTB base class (#2754171: Create a Views and ViewsUI FunctionalJavascriptTestBase class)

Some starting notes on the conversion:

• I tried to stick as close to the original tests as possible, I never changed anything to improve the tests
• I removed several t()'s because they make the test fail
• setUp() needs a param to work
• The contextual test is JavascriptTestBase now because it tested Javascript. The way it's opened is by adding a class because it cannot work in PhantomJS currently (see #2821724: Create Javascript Tests for Contextual Links).

Note: patch contains other 'stuff' (#2862947: Incorrect field assertions in AssertLegacyTrait and ViewTestBase) that needs to be removed before it can be committed.

Thanks very much for the review and kind words!

1,3: Thanks!
2: I inject random 4's into my patches to see if everyone is paying attention ;)
4: Used a string cast, also works!
5: Done!
6: Let's see if the testbot requires them, removed them in this patch. We're having simlar problems in #2863268: Convert web tests to browser tests for tracker module and it only fails on php 7.1.

Furthermore:
- Restored original base classes to make the non-converted tests work
- Still can't get 2 tests to work. One with language problems and 1 with radio buttons. I can't get radio buttons to work, anyone know a place where there are working radio buttons in a BrowserTestBase?

I've been discussing with @lendude offline.

Views and views_ui are important to get in fast because a lot of other conversions will be blocked without them. We both have a small set of tests that are giving us a lot of trouble. I propose to split these off into separate issues so we can get the views and views_ui conversions in.

Does this sound like a good idea and should I start splitting them off?

Yes, I think that makes sense to make progress here.

Ok, cool! Thanks for the reply. I'll split the problematic tests into separate issues and make sure the rest pass on all php versions.

Created new issues for the 2 remaining tests (Updated IS here):
#2864610: Convert ExposedFormUITest in views_ui module to BrowserTestBase
#2864613: Convert ViewEditTest in views_ui module to BrowserTestBase

Will post my work-in-progress there.

New patch excludes these tests, let's see what it does now.

Fixed some incorrect namespaces and the nitpick.

Yeah the negative assertions were broken, really difficult to get those right. The actual fix is in #2862947: Incorrect field assertions in AssertLegacyTrait btw, this patch contains a rough version to make the tests pass. These changes should be removed before commit. The ViewTestBase must also be removed and should use the one @lendude is making in in the views conversion.

@dawehner
I feel confident on the made changes. They are minimal in every step and were all required to get it to run on phpunit.

The ViewTestBase must also be removed and should use the one @lendude is making in in the views conversion.

Yeah the one in #2863267: Convert web tests of views contains some additional tweaks for BrowserTestBase compatibility.

Some wrong namespaces now fixed. Hope it passes, if it does I'll create a clean patch without additions to merge it with views tests.

Green never looked better :) Man it feels good to have this one pass.

Patch without extra's, this one will fail and is only for merging into the views conversion or to be committed after views and the improvement for AssertLegacyTrait is in.

@michielnugter would you mind summarising the issue IDs this requires and marking this as postponed for now. Feels good to be so close to getting this first chunk in!

Updated the IS and postponed the issue.

@michielnugter++

Updated patch with deprecation warning on the old UITestbase.

Moved the WizardTest out of the views scope and into a separate issue: #2867777: Convert views WizardTestBase tests to phpunit.

IS has been updated.

Let's see what this does now views is in :)

Right, slight API changes from the used patch and the one that landed for the field assertions. These are now fixed and the test should go green..

Some feedback/nits:

1. +++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationViewListUiTest.php
   @@ -25,7 +25,7 @@ class ConfigTranslationViewListUiTest extends UITestBase {
   +  protected function setUp($import_test_views = TRUE) {
        parent::setUp();
   

   $import_test_views should be passed to the parent, this is missed on a number of tests

2. +++ b/core/modules/user/tests/src/Functional/AccessRoleUITest.php
   --- a/core/modules/views_ui/src/Tests/UITestBase.php
   +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   
   +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   +++ b/core/modules/views_ui/src/Tests/UITestBase.php
   @@ -2,6 +2,8 @@
   
   @@ -2,6 +2,8 @@
    
    namespace Drupal\views_ui\Tests;
    
   +@trigger_error('\Drupal\views_ui\Tests\UITestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\views_ui\Functional\UITestBase', E_USER_DEPRECATED);
   +
   

   missing @deprecated

3. +++ b/core/modules/views_ui/tests/src/Functional/DisplayAttachmentTest.php
   @@ -16,6 +16,7 @@ class DisplayAttachmentTest extends UITestBase {
   +   * .
   

   ?

4. +++ b/core/modules/views_ui/tests/src/Functional/FieldUITest.php
   @@ -69,10 +68,11 @@ public function testFieldUI() {
   +    // @todo Get this test working under BrowserTestBase.
   +//    $edit_handler_url = '/admin/structure/views/ajax/handler-group/test_view/default/field/name';
   +//    $this->drupalGet($edit_handler_url);
   +//    $data = Json::decode($this->getRawContent());
   +//    $this->assertEqual($data[3]['dialogOptions']['title'], 'Configure aggregation settings for field Views test: Name');
   

   if we have a follow up for this, can we add it to the comment? And in the views conversion I did $data = Json::decode($this->getSession()->getPage()->getContent()); and that seemed to do the trick.

Thanks for the review, back to needs work for that.

And fixed!

Thanks for the hint on FieldTestUI, worked perfectly!

Grr, wrong file..

interdiff contains a remove because I rolled the last patch with --find-copies.

Nittiest of nits:

+++ b/core/modules/views_ui/tests/src/Functional/FieldUITest.php
@@ -69,9 +69,10 @@ public function testFieldUI() {
+    // @todo Get this test working under BrowserTestBase.

Think we got this to work now! So no need for the @todo

Made a bit of a mess I think with my previous patch. I seem to have added deprecation warnings on the new UITestBase class and reverted some of the made changes.

New patch should do everything right and I removed the @todo line :)

Applied the patch and checked that everything gets moved to the right spot and the deprecations are on the right classes. All looks good. Only thing I can think of is moving WizardTest back into scope here. The base class is available now and it passes when simply moved.

Sounds like a good plan, I'll work on this sometime today.

Lots of things changed since the last attempt at ExposedFormUITest and ViewEditTest, lets see if they pass now as well.

Right, still failing (among other fails?).

This one only contains the WizardTest extra since #95 and if ok should be able to get committed.

@michielnugter stop doing too many things at once :)

Pretty sure #101 patch contains a couple of extra lines you didn't want in there, about 30kb worth of them :D

Let's see how I do without the massive headache.. :)

Still don't completely understand the interdiff for UiTestBase but that one keeps beeing weird, it's the same as an older version as far as I can tell.

+++ b/core/modules/views_ui/src/Tests/UITestBase.php
@@ -1,11 +1,16 @@
+@trigger_error('\Drupal\views_ui\Tests\UITestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.x. Instead, use \Drupal\Tests\views_ui\Functional\UITestBase', E_USER_DEPRECATED);

This now needs to go inside the setUp method, see #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

Fixed #105

#106:
Even though it's not ideal in this way this is the smallest change possible. the goal for the conversions up till now has been to keep the changes to a minimum to ease the review and commit process. Not 100% sure here as I already changed this line of code, should we completely fix each line in the tests to not use deprecated methods and the proper ones or stick with the minimum change policy?

I guess I just don't care much enough about that :) We can indeed always come back later and improve things.

I care up to the level that I know what to do on reviews and conversions. I'm strongly towards not changing if it's not needed to make the conversions low impact, I'm not going to set anything back to Needs review though. We'll have to do the fun initiative to remove all deprecated code some other time, the current BrowserTestBase tests are full of deprecated code usage..

Should we postpone this issue for now onto the other one, where we figure out how to actually deprecate test classes?

Agree, let's settle that policy first.

As discussed in #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace the following test needs to either be moved into scope for this issue or a follow-up must be created to convert the ConfigTranslationViewListUiTest test to WebTestBase. Adding Needs followup tag for now.

Turned out that ConfigTranslationViewListUiTest was already in this patch :) Deprecation policy was settled and in line with the current state of this issue.

Needed a re-roll now though as the file was moved.

I think it's ready to go, one final review before RTBC and test-bot run would be very much appreciated :)

Manually checked the patch again.

All moves look good. Follow ups have been opened for the remaining 5 tests. Base class deprecation is good. Lets do this!

Removed the wizardtest from the IS since it's in scope now.

Sorry, jumped the gun, I think we missed \Drupal\taxonomy\Tests\Views\TaxonomyParentUITest

And it's back.

Interdiff still contains garbage, it really doesn't like the --find-copies.

Rechecked, \Drupal\views_ui\Tests\UITestBase now only gets used by 4 classes, and we have followups for all of them.

The rest is still good.

This looks great, really happy to see this one ready.

Committed/pushed to 8.4.x, thanks!

8.3.x patch release today, so leaving RTBC against 8.3.x for cherry-pick either late today or more likely tomorrow.

@catch thanks so much for committing!

And thanks everybody that put time into this one. Really great to see another big chunk go in.

@catch++

Thanks for the commit! Awesome to see it finally getting in after so much work!

Queued the patch for 8.3.x

Stil green then, back to rtbc

Cherry-picked to 8.3.x, thanks!