Comments

nlisgo created an issue. See original summary.

Jo Fitzgerald’s picture

Status: Active » Needs review
FileSize
4.51 KB
  • Moved all 4 files.
  • Corrected namespace.
  • Updated references.

Sorry @nlisgo, I only just noticed you had assigned this to yourself after doing the work.

Status: Needs review » Needs work

The last submitted patch, 2: 2863416-2.patch, failed testing.

nlisgo’s picture

Assigned: nlisgo » Unassigned

@jo-fitzgerald Happy to step away from this and move on to a different module.

zviryatko’s picture

About changes in core/modules/filter/tests/src/Functional/FilterAdminTest.php

assertFieldByName() now deprecated and was changed, so third paramater was removed, also test assert was changed because we need to check that checkboxes, which was checked on add filter form, is checked on filter edit form.

Also $this->xpath() now return NodeElement[] array, instead of \SimpleXMLElement[]|bool, so all function call also changed.

zviryatko’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work

Hi @zviryatko, thank you for your contribution!

First tip: Try to use the following git configuration:

[diff]
renames = copies
This detects renames, see https://www.drupal.org/documentation/git/configure for more details. This will make the patch size significantly smaller.

zviryatko’s picture

FileSize
13.59 KB

Sorry, forgot about this ((

zviryatko’s picture

Status: Needs work » Needs review
michielnugter’s picture

Status: Needs review » Needs work

@zviryatko Thanks for the patch! Great to see this moving forward!

Some review comments:

  1. +++ b/core/modules/filter/tests/src/Functional/FilterAdminTest.php
    @@ -142,7 +142,7 @@ public function testFormatAdmin() {
    +    $this->assertTrue(!empty($edit_link), format_string('Link href %href found.',
    
    @@ -297,8 +297,8 @@ public function testFilterAdmin() {
    +    $this->assertTrue(!empty($view_link), 'The message area contains a link to a node');
    
    +++ b/core/modules/filter/tests/src/Functional/FilterFormTest.php
    @@ -199,14 +196,13 @@ protected function assertNoSelect($id) {
    +    $this->assertTrue(!empty($select), SafeMarkup::format('Field @id exists.', [
    
    @@ -253,14 +247,13 @@ protected function assertRequiredSelectAndOptions($id, array $options) {
    +    $this->assertTrue(!empty($select), SafeMarkup::format('Required field @id exists.', [
    
    @@ -276,8 +269,7 @@ protected function assertEnabledTextarea($id) {
    +    $this->assertTrue(!empty($textarea), SafeMarkup::format('Enabled field @id exists.', [
    
    @@ -295,17 +287,17 @@ protected function assertDisabledTextarea($id) {
    +    $this->assertTrue(!empty($textarea), SafeMarkup::format('Disabled field @id exists.', [
    

    You can use assertNotEmpty here.

  2. +++ b/core/modules/filter/tests/src/Functional/FilterFormTest.php
    @@ -173,13 +173,10 @@ protected function doFilterFormTestAsNonAdmin() {
    +    $this->assertTrue(empty($select), SafeMarkup::format('Field @id does not exist.', [
    
    @@ -253,14 +247,13 @@ protected function assertRequiredSelectAndOptions($id, array $options) {
    +    $this->assertTrue(!empty($select), SafeMarkup::format('Required field @id exists.', [
    

    You can use assertEmpty() here.

  3. +++ b/core/modules/filter/tests/src/Functional/FilterFormatAccessTest.php
    @@ -151,7 +151,7 @@ public function testFormatPermissions() {
    +      $options[(string) $element->getValue()] = $element;
    

    (string) cast is not required anymore

  4. +++ b/core/modules/filter/tests/src/Functional/FilterHtmlImageSecureTest.php
    @@ -141,14 +143,14 @@ public function testImageSource() {
    +          $this->assertEqual((string) $element->getAttribute('src'), $red_x_image);
    +          $this->assertEqual((string) $element->getAttribute('alt'), $alt_text);
    +          $this->assertEqual((string) $element->getAttribute('title'), $title_text);
    +          $this->assertEqual((string) $element->getAttribute('height'), '16');
    +          $this->assertEqual((string) $element->getAttribute('width'), '16');
    ...
    +          $this->assertEqual((string) $element->getAttribute('src'), $converted);
    

    (string) cast is not required anymore

michielnugter’s picture

Issue tags: +phpunit initiative
zviryatko’s picture

FileSize
14.35 KB
11.37 KB
zviryatko’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/filter/tests/src/Functional/FilterAdminTest.php
    @@ -142,7 +142,7 @@ public function testFormatAdmin() {
    -    $this->assertTrue($edit_link, format_string('Link href %href found.',
    +    $this->assertNotEmpty($edit_link, format_string('Link href %href found.',
    

    I really like these changes.

  2. +++ b/core/modules/filter/tests/src/Functional/FilterFormTest.php
    @@ -199,14 +196,13 @@ protected function assertNoSelect($id) {
    -    $select = reset($select);
    -    $passed = $this->assertTrue($select instanceof \SimpleXMLElement, SafeMarkup::format('Field @id exists.', [
    +    $this->assertNotEmpty($select, SafeMarkup::format('Field @id exists.', [
    

    +1 also for this change. IMHO we don't loose any level of test coverage here.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 26ad8f8 to 8.4.x and 589eede to 8.3.x. Thanks!

Committed to 8.3.x since this is tests only.

  • alexpott committed 26ad8f8 on 8.4.x
    Issue #2863416 by zviryatko, Jo Fitzgerald, michielnugter: Convert web...

  • alexpott committed 589eede on 8.3.x
    Issue #2863416 by zviryatko, Jo Fitzgerald, michielnugter: Convert web...

  • alexpott committed 1394be5 on 8.4.x
    Revert "Issue #2863416 by zviryatko, Jo Fitzgerald, michielnugter:...

  • alexpott committed fe12420 on 8.3.x
    Revert "Issue #2863416 by zviryatko, Jo Fitzgerald, michielnugter:...
alexpott’s picture

Status: Fixed » Needs work
michielnugter’s picture

Assigned: Unassigned » michielnugter
Status: Needs work » Needs review
FileSize
846 bytes
14.32 KB

Sorry about that @alexpott, didn't catch that one at review.

Updated the patch to conform to the new AssertLegacyTrait, this should pass.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in

  • alexpott committed facaf22 on 8.4.x
    Issue #2863416 by zviryatko, michielnugter, Jo Fitzgerald: Convert web...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed facaf22 to 8.4.x and d002aa9 to 8.3.x. Thanks!

  • alexpott committed d002aa9 on 8.3.x
    Issue #2863416 by zviryatko, michielnugter, Jo Fitzgerald: Convert web...

  • alexpott committed 9d6de39 on 8.4.x
    Revert "Issue #2863416 by zviryatko, michielnugter, Jo Fitzgerald:...
alexpott’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
14.35 KB

I'm really sorry - additional problems with #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions came to light so we had to revert it - which means we have to revert this too.

Re-uploading #12 as that worked before and will commit if it is green.

alexpott’s picture

michielnugter’s picture

Status: Reviewed & tested by the community » Needs work

Discussed with @alexpott on IRC. We'll have to fix the checkbox assertion that currently is failing to a correct test using the non-deprecated methods of Mink.

$this->assertFieldByName('roles[' . RoleInterface::AUTHENTICATED_ID . ']', RoleInterface::AUTHENTICATED_ID);

This assertion is incorrect because it checks if the value is equal to a role ID. Reading the test what was attempted to assert was if the checkbox was checked or not, this assertion does not test this at all. At most it tests if the checkbox is rendered properly with a value matching the name.

Without using the deprecated methods you could assert the state of a checkbox like this:

$this->assertSession()->checkboxChecked($id);

or

$field = $this->getSession()->getPage()->findField('name');
$this->assertTrue($field->isChecked();
naveenvalecha’s picture

As we have checkbox names available. So we could use $this->assertSession()->checkboxChecked

//Naveen

michielnugter’s picture

Status: Needs review » Reviewed & tested by the community

@naveenvalecha thanks for the patch!

Looks good, setting to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2863416-30.patch, failed testing. View results

Mile23’s picture

naveenvalecha’s picture

Status: Needs work » Reviewed & tested by the community

back to RTBC per #31

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

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

  • catch committed 32d9c06 on 8.5.x
    Issue #2863416 by zviryatko, michielnugter, naveenvalecha, alexpott, Jo...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 36e30f1 on 8.4.x
    Issue #2863416 by zviryatko, michielnugter, naveenvalecha, alexpott, Jo...
catch’s picture

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

Status: Fixed » Closed (fixed)

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