Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Sorry for being late here. I was busy with my practicals exams. Assigning back to myself.
This test also includes the patch #2750941: Additional BC assertions from WebTestBase to BrowserTestBase as it depends on couple of assertion helpers of that module.
This patch will fails let's see what ci throws.
How about the tests that are extending viewsTestBase, should we cover this in chunks OR rely on views module conversion first ? how are you dealing with those ? cc / @dawehner / @klausi ?
CreditAttribution: dawehner as a volunteer commented
How about the tests that are extending viewsTestBase, should we cover this in chunks OR rely on views module conversion first ? how are you dealing with those ? cc / @dawehner / @klausi ?
I totally believe that we should have a dedicated issue for those issues.
This assertion is failing Failed asserting that false is true. Other ContactPersonalTest.php 111 Drupal\Tests\contact\Functional\ContactPersonalTest->testPersonalContactAccess()
Did not find the exact reason of this failure.
The conversion of assertUniqueTextHelper is quite confusing here.
Mink does not provide any helper function out of the box to find the unique text on the page and we can't access the getPage() method of Webassert directly in AssertLegacyTrait. So not sure how to proceed with that.We have only responsePageContains/pageTextContains method exists in it and we have getTextContent method exists in BrowserTestBase
Here's the chunks of code that I tried :
/**
* Passes if the text is found MORE THAN ONCE on the text version of the page.
*
* The text version is the equivalent of what a user would see when viewing
* through a web browser. In other words the HTML has been filtered out of
* the contents.
*
* @param string|\Drupal\Component\Render\MarkupInterface $text
* Plain text to look for.
* @param string $message
* (optional) A message to display with the assertion. Do not translate
* messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed
* variables in the message text, not t(). If left blank, a default message
* will be displayed.
* @param string $group
* (optional) The group this message is in, which is displayed in a column
* in test output. Use 'Debug' to indicate this is debugging output. Do not
* translate this string. Defaults to 'Other'; most tests do not override
* this default.
*
* @return bool
* TRUE on pass, FALSE on fail.
*
* @deprecated Scheduled for removal in Drupal 9.0.0.
* Use $this->assertSession()->responseContains() or
* $this->assertSession()->pageTextContains() instead.
*/
protected function assertNoUniqueText($text, $message = '', $group = 'Other') {
return $this->assertUniqueTextHelper($text, $message, $group, FALSE);
}
/**
* Helper for assertUniqueText and assertNoUniqueText.
*
* It is not recommended to call this function directly.
*
* @param string|\Drupal\Component\Render\MarkupInterface $text
* Plain text to look for.
* @param string $message
* (optional) A message to display with the assertion. Do not translate
* messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed
* variables in the message text, not t(). If left blank, a default message
* will be displayed.
* @param string $group
* (optional) The group this message is in, which is displayed in a column
* in test output. Use 'Debug' to indicate this is debugging output. Do not
* translate this string. Defaults to 'Other'; most tests do not override
* this default. Defaults to 'Other'.
* @param bool $be_unique
* (optional) TRUE if this text should be found only once, FALSE if it
* should be found more than once. Defaults to FALSE.
*
* @return bool
* TRUE on pass, FALSE on fail.
*
* @deprecated Scheduled for removal in Drupal 9.0.0.
* Use $this->assertSession()->responseContains() or
* $this->assertSession()->pageTextContains() instead.
*/
protected function assertUniqueTextHelper($text, $message = '', $group = 'Other', $be_unique = FALSE) {
// Cast MarkupInterface objects to string.
$text = (string) $text;
if (!$message) {
$message = '"' . $text . '"' . ($be_unique ? ' found only once' : ' found more than once');
}
$content_type = $this->getSession()->getResponseHeader('Content-type');
// In case of a Non-HTML response (example: XML) check the original
// response.
if (strpos($content_type, 'html') === FALSE) {
$this->assertSession()->responseContains($text);
}
else {
$this->assertSession()->pageTextContains($text);
}
$first_occurrence = strpos($this->getTextContent(), $text);
if ($first_occurrence === FALSE) {
return $this->assert(FALSE, $message, $group);
}
$offset = $first_occurrence + strlen($text);
$second_occurrence = strpos($this->getTextContent(), $text, $offset);
return $this->assert($be_unique == ($second_occurrence === FALSE), $message, $group);
}
Unassinging from myself until i get some clue/feedback on how to achieve / proceed with this.
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -57,6 +58,9 @@ protected function assertElementNotPresent($css_selector) {
protected function assertText($text) {
+ // Cast MarkupInterface to string.
+ $text = (string) $text;
@@ -64,6 +68,10 @@ protected function assertText($text) {
+ // Mink/BrowserKit uses PHP's DOMElement to get the value of an HTML tag.
+ // It will provide the value HTML entity decoded, so we need to do the
+ // same here for the text to actually match.
+ $text = html_entity_decode($text);
@@ -82,6 +90,9 @@ protected function assertText($text) {
protected function assertNoText($text) {
+ // Cast MarkupInterface to string.
+ $text = (string) $text;
@@ -89,6 +100,10 @@ protected function assertNoText($text) {
+ // Mink/BrowserKit uses PHP's DOMElement to get the value of an HTML tag.
+ // It will provide the value HTML entity decoded, so we need to do the
+ // same here for the text to actually match.
+ $text = html_entity_decode($text);
+++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
@@ -154,6 +155,52 @@ public function testAssertOptionSelectedFail() {
/**
+ * @covers ::assertText
+ */
+ public function testAssertText() {
...
+ /**
+ * @covers ::assertNoText
+ */
+ public function testAssertNoText() {
+++ b/core/tests/Drupal/Tests/DocumentElement.php
@@ -0,0 +1,46 @@
+/**
+ * Overrides the Mink document to better handle page text.
+ */
+class DocumentElement extends MinkDocumentElement {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getText() {
So, the parent getText() doesn't remove style and script? Also here the output will contain encoded entities while parent::getText() return decoded entities (as a human would read them). See #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3). I think we should get first parent::getText() and apply changes against that. I'm not sure about the solution and, as we did in every conversion, this should be split in other issue.
As I said, I'm uncertain of the correct file locations, can you give me a hand @klausi? Which files should be where in this case? That should help me for future issues too. Thanks
+++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
@@ -386,14 +388,14 @@ function testAutoReply() {
diff --git a/core/modules/contact/src/Tests/ContactStorageTest.php b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
diff --git a/core/modules/contact/src/Tests/ContactStorageTest.php b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
similarity index 98%
similarity index 98%
rename from core/modules/contact/src/Tests/ContactStorageTest.php
rename from core/modules/contact/src/Tests/ContactStorageTest.php
rename to core/modules/contact/tests/src/Functional/ContactStorageTest.php
A rename is not enough, you also need to change the namespace in this file.
+++ b/core/modules/contact/tests/src/Functional/ContactStorageTest.php
@@ -1,6 +1,6 @@
diff --git a/core/modules/contact/src/Tests/Update/ContactUpdateTest.php b/core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
diff --git a/core/modules/contact/src/Tests/Update/ContactUpdateTest.php b/core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
similarity index 97%
similarity index 97%
rename from core/modules/contact/src/Tests/Update/ContactUpdateTest.php
rename from core/modules/contact/src/Tests/Update/ContactUpdateTest.php
rename to core/modules/contact/tests/src/Functional/Update/ContactUpdateTest.php
I think the update path tests are special and we might not be able to convert them in this issue.
+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -36,11 +36,19 @@ public function testGoTo() {
// Test page contains some text.
$this->assertSession()->pageTextContains('Test page text.');
+ // Make sure CSS <style> content is not considered as part of the page text.
+ $this->assertSession()->pageTextNotContains('@import');
+ // Make sure JS setting values are not part of the page text.
+ $this->assertSession()->pageTextNotContains('"baseUrl"');
unrelated change, we should not change BrowserTestBaseTest in this issue.
+++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
index 0000000..323bde2
--- /dev/null
--- /dev/null
+++ b/core/tests/Drupal/Tests/DocumentElement.php
same here, this is all coming from other patches form other issues and should not be included in this patch.
The namespace was changed as part of #35, is namespace Drupal\Tests\contact\Functional; not correct?
I have moved Update/ContactUpdateTest.php back to where is was.
These changes came from the patch in #29, as mentioned on another ticket, should I not be basing my patches on the existing patches? Or should I just start from scratch and ignore previous patches?
bighappyfaceCreditAttribution: bighappyface as a volunteer commented
I can reproduce the exception for Drupal\Tests\contact\Functional\ContactSitewideTest
$ php ./core/scripts/run-tests.sh --url http://127.0.0.1:8888 --verbose --color --class "Drupal\Tests\contact\Functional\ContactSitewideTest"
Drupal test run
---------------
Tests to be run:
- Drupal\Tests\contact\Functional\ContactSitewideTest
Test run started:
Friday, February 24, 2017 - 11:25
Test summary
------------
Drupal\Tests\contact\Functional\ContactSitewideTest 0 passes 1 exceptions
FATAL Drupal\Tests\contact\Functional\ContactSitewideTest: test runner returned a non-zero error code (2).
Drupal\Tests\contact\Functional\ContactSitewideTest 0 passes 1 fails
Test run duration: 1 min 21 sec
Detailed test results
---------------------
---- Drupal\Tests\contact\Functional\ContactSitewideTest ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Exception Other phpunit-9.xml 0 Drupal\Tests\contact\Functional\Con
PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian
Bergmann and contributors.
EE
Time: 1.34 minutes, Memory: 7.00Mb
There were 2 errors:
1) Drupal\Tests\contact\Functional\ContactSitewideTest::testSiteWideContact
Undefined property: Behat\Mink\Element\NodeElement::$td
/workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php:272
2) Drupal\Tests\contact\Functional\ContactSitewideTest::testAutoReply
PHPUnit_Framework_Exception: Fatal error: Call to undefined method
Drupal\Tests\contact\Functional\ContactSitewideTest::drupalGetMails() in
/workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
on line 409
bighappyfaceCreditAttribution: bighappyface as a volunteer commented
Same exception verified for Drupal\Tests\contact\Functional\ContactStorageTest
$ php ./core/scripts/run-tests.sh --url http://127.0.0.1:8888 --verbose --color --class "Drupal\Tests\contact\Functional\ContactStorageTest"
Drupal test run
---------------
Tests to be run:
- Drupal\Tests\contact\Functional\ContactStorageTest
Test run started:
Friday, February 24, 2017 - 11:30
Test summary
------------
Drupal\Tests\contact\Functional\ContactStorageTest 0 passes 1 exceptions
FATAL Drupal\Tests\contact\Functional\ContactStorageTest: test runner returned a non-zero error code (2).
Drupal\Tests\contact\Functional\ContactStorageTest 0 passes 1 fails
Test run duration: 1 min 45 sec
Detailed test results
---------------------
---- Drupal\Tests\contact\Functional\ContactStorageTest ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Exception Other phpunit-10.xml 0 Drupal\Tests\contact\Functional\Con
PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian
Bergmann and contributors.
.EE
Time: 1.73 minutes, Memory: 7.25Mb
There were 2 errors:
1) Drupal\Tests\contact\Functional\ContactStorageTest::testSiteWideContact
Undefined property: Behat\Mink\Element\NodeElement::$td
/workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php:272
2) Drupal\Tests\contact\Functional\ContactStorageTest::testAutoReply
PHPUnit_Framework_Exception: Fatal error: Call to undefined method
Drupal\Tests\contact\Functional\ContactStorageTest::drupalGetMails() in
/workspace/drupal-8.4.x/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
on line 409
Comments
Comment #2
naveenvalechaComment #3
naveenvalechaassigning to myself. will work on it tonight
Comment #4
naveenvalechaComment #5
dawehnerLet's unassign, so someone else could pick it up.
@naveenvalecha of course you can pick it up at any point in time :)
Comment #6
naveenvalechaSorry for being late here. I was busy with my practicals exams. Assigning back to myself.
This test also includes the patch #2750941: Additional BC assertions from WebTestBase to BrowserTestBase as it depends on couple of assertion helpers of that module.
This patch will fails let's see what ci throws.
How about the tests that are extending viewsTestBase, should we cover this in chunks OR rely on views module conversion first ? how are you dealing with those ? cc / @dawehner / @klausi ?
Comment #7
dawehnerLet's do that in their own subissue. They will probably take some time, given the speciality of views.
Comment #9
dawehnerI totally believe that we should have a dedicated issue for those issues.
This code in
ContactSidewideTest
is now problematic, asclickLink()
no longer supports$i
.Note: There are methods which no longer support a $message, like.
$this->assertResponse(200, 'The page exists');
Let's drop those unused parameters.
Let's rather rename the usages
PS: @naveenvalecha Do you mind including review only patches? They are really helpful to focus.
Comment #10
naveenvalechaIncluded the patch https://www.drupal.org/node/2750941#comment-11320591
Addressed :
#9.2
This assertion is failing
Failed asserting that false is true. Other ContactPersonalTest.php 111 Drupal\Tests\contact\Functional\ContactPersonalTest->testPersonalContactAccess()
Did not find the exact reason of this failure.
The conversion of assertUniqueTextHelper is quite confusing here.
Mink does not provide any helper function out of the box to find the unique text on the page and we can't access the getPage() method of Webassert directly in AssertLegacyTrait. So not sure how to proceed with that.We have only responsePageContains/pageTextContains method exists in it and we have getTextContent method exists in BrowserTestBase
Here's the chunks of code that I tried :
Unassinging from myself until i get some clue/feedback on how to achieve / proceed with this.
Comment #11
naveenvalechaComment #12
dawehner@naveenvalecha
I think the right strategy is to add the helper function to WebAssert in Drupal itself ...
Comment #14
claudiu.cristeaPartial work. Still fails because it seems that \Behat\Mink\WebAssert::pageTextNotContains() is not correctly extracting the text from the HTML page.
Comment #16
dawehnerThank you for this patch!
Please put
into your git config, see https://www.drupal.org/documentation/git/configure
Also note: This patch includes a book module change, even this issue is just about contact module :)
Comment #17
claudiu.cristea@dawehner, yes I noticed. The patch is not usable. Sorry, I'll get back.
Comment #18
claudiu.cristeaHere's the correct patch. interdiff is OK from #14
Comment #22
naveenvalechaThe 2742995-22.patch also used
assertUniqueText
from this patch and its also baked into it. https://www.drupal.org/node/2759879#comment-11419115Also it added a new method
assertCacheContext
to AssertLegacytrait.phpComment #24
klausiwe should rename the methods same as SimpletTest does so that we don't need to change this line.
we should add the assertNoFieldChecked() method to the AssertLegacy trait instead and not change this line.
same here.
I think for maximum compatibility we should add AssertMailTRait to BrowserTestBase instead to keep parity with SimpleTest.
use assertCotains() instead.
Comment #25
dawehnerWell we should have some BC layer in place, but IMHO putting Drupal on there, just conceptually doesn't make sense.
Comment #27
klausiRerolled and some progress, but this will still fail. Contains:
* #2784263: Move WTB::assertCacheContext() to AssertPageCacheContextsAndTagsTrait
* #2784427: Browser tests: Plain page text contains CSS styles and JS settings
* #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3)
Comment #29
klausiAnd we also need #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests for this conversion patch, rolled that in.
Now all contact module tests are converted, except for Views tests and Update Path tests.
Comment #31
claudiu.cristeaShouldn't be here. Part of #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests.
These changes were deferred to #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).
So, the parent getText() doesn't remove style and script? Also here the output will contain encoded entities while parent::getText() return decoded entities (as a human would read them). See #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3). I think we should get first parent::getText() and apply changes against that. I'm not sure about the solution and, as we did in every conversion, this should be split in other issue.
Postponing on #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests and #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).
Comment #32
naveenvalechabetter title
Comment #34
jibranComment #35
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #36
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll. I'm not 100% certain of the file locations - the previous patch was noticeably different to 8.4.x.
Comment #37
klausiLooks like you forgot to move the files to the tests directory?
Comment #38
jofitz CreditAttribution: jofitz at ComputerMinds commentedAs I said, I'm uncertain of the correct file locations, can you give me a hand @klausi? Which files should be where in this case? That should help me for future issues too. Thanks
Comment #39
klausithey should be in tests/src/Functional of the module. See for example https://www.drupal.org/commitlog/commit/2/4c64f7b840ed4cbd1d075b3f409225...
Comment #40
jofitz CreditAttribution: jofitz at ComputerMinds commentedHow about this then?
Comment #41
klausiA rename is not enough, you also need to change the namespace in this file.
I think the update path tests are special and we might not be able to convert them in this issue.
unrelated change, we should not change BrowserTestBaseTest in this issue.
same here, this is all coming from other patches form other issues and should not be included in this patch.
Comment #42
jofitz CreditAttribution: jofitz at ComputerMinds commentednamespace Drupal\Tests\contact\Functional;
not correct?Comment #44
jofitz CreditAttribution: jofitz at ComputerMinds commentedViews/
tests back toTests/Views/
.Comment #46
bighappyface CreditAttribution: bighappyface as a volunteer commentedI can reproduce the exception for
Drupal\Tests\contact\Functional\ContactSitewideTest
Comment #47
bighappyface CreditAttribution: bighappyface as a volunteer commentedSame exception verified for
Drupal\Tests\contact\Functional\ContactStorageTest
Comment #48
claudiu.cristeaComment #50
claudiu.cristeaMore clear.
Comment #51
bighappyface CreditAttribution: bighappyface as a volunteer commented+1 LGTM - #50 verified
Comment #53
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #54
jofitz CreditAttribution: jofitz at ComputerMinds commentedSetting back to RTBC in expectation.
Comment #56
jofitz CreditAttribution: jofitz at ComputerMinds commentedI forgot to move the files.
Comment #57
klausiLooks good, thanks!
Comment #58
alexpottCommitted and pushed 9e85f54 to 8.4.x and 6755fc9 to 8.3.x. Thanks!
Comment #62
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commented