See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Tests moved:
EntityTypeWithoutLanguageFormTest.php
LanguageConfigurationElementTest.php
LanguageConfigurationTest.php
LanguageCustomLanguageConfigurationTest.php
LanguageListTest.php
LanguageLocaleListTest.php
LanguageSelectorTranslatableTest.php
LanguageSwitchingTest.php
LanguageUILanguageNegotiationTest.php
LanguageUrlRewritingTest.php

Tests not moved:
LanguageTourTest.php => #2871740: Convert web tests to browser tests LanguageTourTest for language module
LanguageSelectWidgetUpdateTest.php => For now #2808777: Research: Complex tests to convert

CommentFileSizeAuthor
#46 interdiff-2756059-43-46.txt284 bytesGoZ
#46 convert_web_tests_to-2756059-46.patch23.33 KBGoZ
#43 interdiff-2756059-41-43.txt3.6 KBGoZ
#43 convert_web_tests_to-2756059-43.patch24.06 KBGoZ
#41 convert_web_tests_to-2756059-41.patch20.76 KBGoZ
#39 convert_web_tests_to-2756059-39.patch24.01 KBGoZ
#39 interdiff-2756059-37-39.txt1.18 KBGoZ
#37 interdiff-2756059-32-37.txt668 bytesGoZ
#37 convert_web_tests_to-2756059-37.patch23.68 KBGoZ
#32 2756059-32.patch21.87 KBjofitz
#32 interdiff-27-32.txt12.96 KBjofitz
#32 interdiff-25-32.txt8.63 KBjofitz
#27 2756059-27.patch11.29 KBjofitz
#27 interdiff-26-27.txt18.52 KBjofitz
#27 2756059-26.patch11.29 KBjofitz
#20 browsertest-language-2756059-20.patch40.01 KBklausi
#11 interdiff.txt11.52 KBklausi
#11 browsertest-language-2756059-11.patch54.17 KBklausi
#9 interdiff.txt3.34 KBklausi
#9 browsertest-language-2756059-9.patch45.63 KBklausi
#7 interdiff.txt13.38 KBklausi
#7 browsertest-language-2756059-7.patch44.09 KBklausi
#4 interdiff-2756059-2-4.txt12.16 KBnaveenvalecha
#4 2756059-4.patch50.74 KBnaveenvalecha
#2 browsertest-language-2756059-2.patch62 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
62 KB

Contains #2750941: Additional BC assertions from WebTestBase to BrowserTestBase and some other refactorings that I will move to dedicated issues later.

Status: Needs review » Needs work

The last submitted patch, 2: browsertest-language-2756059-2.patch, failed testing.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
50.74 KB
12.16 KB

Status: Needs review » Needs work

The last submitted patch, 4: 2756059-4.patch, failed testing.

klausi’s picture

AsserLegacyTrait contains a duplicated method now. Make sure to run at least one converted test to catch PHP fatal errors before posting the patch.

+++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
@@ -39,7 +39,8 @@ function testLanguageList() {
-    $this->assertText('French', 'Language added successfully.');
+    // Language added successfully.
+    $this->assertText('French');

This change is not necessary. Let's leave those calls if they don't break.

klausi’s picture

Status: Needs work » Needs review
FileSize
44.09 KB
13.38 KB

Fixed one more test, this will still fail.

Status: Needs review » Needs work

The last submitted patch, 7: browsertest-language-2756059-7.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
45.63 KB
3.34 KB

And another round. There was a more complicated fix in a test case that worked by submitting invalid form data, which does not work with mink. Implemented that in a nicer way by creating the language entity over the UI and then silently deleting it in the background.

Still not done yet.

Status: Needs review » Needs work

The last submitted patch, 9: browsertest-language-2756059-9.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
54.17 KB
11.52 KB

This should fix the rest. Integrated the Drupal javascript settings method from #2753179: Convert all color web tests to BrowserTestBase.

klausi’s picture

klausi’s picture

klausi’s picture

klausi’s picture

Mile23’s picture

Status: Needs review » Postponed

Applying the patch gives us this, which is out of scope:

	modified:   core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
	modified:   core/tests/Drupal/KernelTests/KernelTestBase.php
	modified:   core/tests/Drupal/Tests/BrowserTestBase.php
	modified:   core/tests/Drupal/Tests/WebAssert.php

So I'm guessing the other trait issues will make those changes and this will eventually need re-work.

Should we postpone on those?

klausi’s picture

Agreed, let's postpone on those.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Eric_A’s picture

Status: Postponed » Needs work
Issue tags: +Needs reroll

Agreed, let's postpone on those.

All in, now.

klausi’s picture

klausi’s picture

Issue tags: +Drupalaton
dawehner’s picture

+++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
@@ -176,9 +176,20 @@ function testLanguageList() {
+    // First create the NL language.
+    $edit = array(
+      'predefined_langcode' => 'nl',
+    );
+    $this->drupalPostForm('admin/config/regional/language/add', $edit, 'Add language');
+
+    // Load the form which has now the additional NL language option.
     $this->drupalGet('admin/config/regional/language');
-    $extra_values = '&site_default_language=nl';
-    $this->drupalPostForm(NULL, array(), t('Save configuration'), array(), array(), NULL, $extra_values);
+
+    // Delete the NL language in the background.
+    $language_storage = $this->container->get('entity_type.manager')->getStorage('configurable_language');
+    $language_storage->load('nl')->delete();
+
+    $this->drupalPostForm(NULL, array('site_default_language' => 'nl'), 'Save configuration');

This change is a bit weird. This language wasn't needed before

dawehner’s picture

Status: Needs review » Needs work

Certainly needs work based upon #22

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jofitz’s picture

Issue tags: +Needs reroll
GoZ’s picture

LanguageTourTest will work with BTB after #2767275: Convert web tests to browser tests for tour module

I replace

+++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
@@ -176,9 +176,20 @@ function testLanguageList() {
+    // First create the NL language.
+    $edit = array(
+      'predefined_langcode' => 'nl',
+    );
+    $this->drupalPostForm('admin/config/regional/language/add', $edit, 'Add language');
+
+    // Load the form which has now the additional NL language option.
     $this->drupalGet('admin/config/regional/language');
-    $extra_values = '&site_default_language=nl';
-    $this->drupalPostForm(NULL, array(), t('Save configuration'), array(), array(), NULL, $extra_values);
+
+    // Delete the NL language in the background.
+    $language_storage = $this->container->get('entity_type.manager')->getStorage('configurable_language');
+    $language_storage->load('nl')->delete();
+
+    $this->drupalPostForm(NULL, array('site_default_language' => 'nl'), 'Save configuration');

by

    // Ensure that NL cannot be set default when it's not available.
    $this->drupalGet('admin/config/regional/language');
    try {
      $this->drupalPostForm(NULL, array('site_default_language' => 'nl'), t('Save configuration'));
      $this->fail('Form validation InvalidArgumentException thrown but not caught.');
    }
    catch (\InvalidArgumentException $e) {
      $this->assertEquals($e->getMessage(), 'Input "site_default_language" cannot take "nl" as a value (possible values: xx).');
    }
    $this->assertFieldChecked('edit-site-default-language-xx', 'The previous default language still selected.');

Original code doesn't work anymore.

I can't do interdiff because #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits already move some files and i cannot apply previous patch without fails.

jofitz’s picture

Remove changes that are not relevant to this issue.

jofitz’s picture

The last submitted patch, 27: 2756059-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2756059-27.patch, failed testing.

jofitz’s picture

Sorry for making this issue so messy. I tried. I failed.

jofitz’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
12.96 KB
21.87 KB

Addressed test failures.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I went through the changes and they look alright!

+++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
@@ -177,8 +180,7 @@ function testLanguageList() {
-    $extra_values = '&site_default_language=nl';
-    $this->drupalPostForm(NULL, array(), t('Save configuration'), array(), array(), NULL, $extra_values);
+    $this->drupalPostForm(NULL, array('site_default_language' => 'nl'), t('Save configuration'));

I guess this was simply not needed before.

jofitz’s picture

@dawehner FYI: That change is because the BTB version of drupalPostForm() has slightly different parameters to the WTB version.

dawehner’s picture

Fair :)

klausi’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
@@ -23,6 +23,9 @@ class LanguageListTest extends WebTestBase {
+   *
+   * @expectedException \InvalidArgumentException
+   * @expectedExceptionMessage Input "site_default_language" cannot take "nl" as a value (possible values: xx).

no, we are not testing exceptions here. We want to test the actual page response here, so those annotations should not be added.

GoZ’s picture

Status: Needs work » Needs review
FileSize
23.68 KB
668 bytes

Status: Needs review » Needs work

The last submitted patch, 37: convert_web_tests_to-2756059-37.patch, failed testing.

GoZ’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
24.01 KB

As done in #26, i try catch \InvalidArgumentException because we cannot send not existed site_default_language like before.

I also keep @exception annotations removed which make tests success in case Exception is catched, which is not the purpose.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/tests/src/Functional/LanguageConfigurationTest.php
    @@ -169,10 +169,7 @@ function testLanguageConfigurationWeight() {
    -    $edit = array(
    -      'confirm' => 1,
    -    );
    -    $this->drupalPostForm('admin/config/regional/language/delete/fr', $edit, 'Delete');
    +    $this->drupalPostForm('admin/config/regional/language/delete/fr', array(), 'Delete');
    

    this introduces long array syntax, but we only allow short array syntax now.

  2. +++ b/core/modules/language/tests/src/Functional/LanguageListTest.php
    @@ -177,10 +177,13 @@ function testLanguageList() {
         // Ensure that NL cannot be set default when it's not available.
         $this->drupalGet('admin/config/regional/language');
    -    $extra_values = '&site_default_language=nl';
    -    $this->drupalPostForm(NULL, array(), t('Save configuration'), array(), array(), NULL, $extra_values);
    -    $this->assertText(t('Selected default language no longer exists.'));
    -    $this->assertNoFieldChecked('edit-site-default-language-xx', 'The previous default language got deselected.');
    +    try {
    +      $this->drupalPostForm(NULL, array('site_default_language' => 'nl'), t('Save configuration'));
    +    }
    +    catch (\InvalidArgumentException $e) {
    +      $this->assertEquals($e->getMessage(), 'Input "site_default_language" cannot take "nl" as a value (possible values: xx).');
    +    }
    +    $this->assertFieldChecked('edit-site-default-language-xx', 'The previous default language still selected.');
    

    Hm, but this is changing the test? In the old test the last assertion was that the field is not checked, but now it is? That's probably because a POST request was never triggered (it was stopped by the InvalidArgumentException).

    Can you go back to my original solution in #20? I know dawehner was against it, but I think it is still better than not doing the POST request. We could also issue an artificial POST request with Guzzle, but I like my approach better because it makes it clear what is going on from a user perspective.

GoZ’s picture

Status: Needs work » Needs review
FileSize
20.76 KB

I cannot generate interdiff since 8.4.x add to much changes in code.

I fix some array to short array.
I came back to solution in #20.

klausi’s picture

Status: Needs review » Needs work
diff --git a/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
index 97ccf2f..f7b6191 100644
--- a/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
+++ b/core/modules/language/src/Tests/EntityTypeWithoutLanguageFormTest.php
@@ -1,8 +1,8 @@
 <?php

Looks like you forgot to move the test files to their new destination. Can you make sure that all modified test files are in the correct place before you upload the next patch? Thanks!

GoZ’s picture

Status: Needs work » Needs review
FileSize
24.06 KB
3.6 KB

Sorry for the mess.

What to do with core/modules/language/src/Tests/Update/LanguageSelectWidgetUpdateTest.php ?
Does the conversion of this file should be done in another issue or in this one ?

klausi’s picture

Status: Needs review » Postponed
+++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
index b11006e..b0dc5d7 100644
--- a/core/modules/language/src/Tests/LanguageTourTest.php

--- a/core/modules/language/src/Tests/LanguageTourTest.php
+++ b/core/modules/language/tests/src/Functional/LanguageTourTest.php

we cannot move that file yet because the TourTestBase has not been converted yet.

Postponing on #2767275: Convert web tests to browser tests for tour module.

Yes, the UpdatePathTestBase are fine to leave untouched until we make progress with #2808777: Research: Complex tests to convert.

michielnugter’s picture

Status: Postponed » Needs work

We can split the Tour test into a separate issue and try to get the rest for the Language module ready, setting back to needs work for that.

GoZ’s picture

Lendude’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Manually checked if everything was moved, updated the IS to reflect the changes done and the follow ups.

All previous feedback has been addressed and I see no issues with the current changes.

michielnugter’s picture

Issue tags: +phpunit initiative

  • catch committed c8b4da1 on 8.4.x
    Issue #2756059 by GoZ, klausi, Jo Fitzgerald, naveenvalecha, dawehner:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

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