Problem/Motivation

The current field assertions are not fully compatible with the Simpletest assertions. This is needed to make the largest possible amount of WebTest conversions pass with simple conversions.

Example of incompatibility:
The current assertFieldByName() and assertNoFieldByName() both only check 1 field on the AssertLegacyTrait class. The old Simpletest method tested all fields on the expected value.

This causes problems when checking on a field that is on the page more than one time, the op field for example.

Second example:
For checkbox fields the current assertFieldById() passes when using a second parameter value of the empty string '' for 'ignore the value, just check if the field exists'. This behavior is incorrect and if you want to ignore the checkbox value the second parameter must be NULL not an empty string.

Proposed resolution

Change the implementations on AssertLegacyTrait to work similar to the implementation of Simpletest.

Remaining tasks

This issue had been postponed, waiting on #2876357: Incomplete test coverage on AssertLegacyTrait field assertions

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#93 2868019-89.patch17.46 KBLendude
#91 2868019-91_including_2867154_31_form_test_conversions.patch61.51 KBjonathan1055
#89 2868019-89.patch17.46 KBLendude
#89 interdiff-2868019-87-89.txt594 bytesLendude
#87 2868019-87.patch17.46 KBLendude
#87 interdiff-2868019-81-87.txt777 bytesLendude
#82 interdiff-2868019-60-81.txt2.34 KBLendude
#81 2868019-81.patch17.33 KBLendude
#81 2868019-81-SHOULD_FAIL.patch17.18 KBLendude
#60 2868019-interdiff-54-60.txt7.03 KBjonathan1055
#60 2868019-60.patch16.81 KBjonathan1055
#59 2868019-59.legacy-tests-only.patch3.39 KBjonathan1055
#57 2868019-56.tests-only.patch4.53 KBjonathan1055
#54 2868019-54.patch13.27 KBLendude
#54 interdiff-2868019-50-54.txt3.15 KBLendude
#50 2868019-50.patch11.12 KBmichielnugter
#50 interdiff-45-50.txt8.13 KBmichielnugter
#47 2868019-47.patch9.37 KBjonathan1055
#46 2868019-46.patch7.36 KBjonathan1055
#45 2868019-45.patch13.43 KBmichielnugter
#45 interdiff-43-45.txt874 bytesmichielnugter
#43 2868019-43.patch13.5 KBmichielnugter
#43 interdiff-39-43.txt1.41 KBmichielnugter
#39 2868019-39.patch14.45 KBmichielnugter
#39 interdiff-21-39.txt1.35 KBmichielnugter
#21 2868019-21.patch14.33 KBmichielnugter
#21 interdiff-18-21.txt3.99 KBmichielnugter
#18 2868019-18.patch13.2 KBmichielnugter
#18 interdiff-14-18.txt6.66 KBmichielnugter
#14 2868019-14.patch11.32 KBmichielnugter
#14 interdiff-7-14.txt2.26 KBmichielnugter
#13 2868019-13.patch11.09 KBmichielnugter
#13 interdiff-7-13.txt3.68 KBmichielnugter
#7 2868019-7.patch9.84 KBmichielnugter
#7 2868019-7-test-only.patch2.64 KBmichielnugter
#4 2868019-4-test-only.patch1.92 KBmichielnugter
#2 2868019-2.patch1.99 KBmichielnugter
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Work in progress, just a first attempt and doesn't work yet.

dawehner’s picture

Ideally we would have some test case first. You know, this kinda gets you the freedom to experiment a bit more :)

michielnugter’s picture

Test driven development, sound like a plan!

Patch now only contains the test, won't even try a needs review because it will fail by design.

It actually failed where I did not fully expect it, it cannot find any field when more are on the page. I'll experiment on it and come up with something that will pass.

As this might be a blocker for many other conversions and a bug in a much used Trait, should it be Major or is normal ok?

michielnugter’s picture

Status: Active » Needs review

Well, maybe a needs review after all to prove it will fail..

Status: Needs review » Needs work

The last submitted patch, 4: 2868019-4-test-only.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
9.84 KB

This kind-of escalated a bit, please tell me if I've been seeing this too big. All the field assertions were actually not fully compatible with the old versions. All were converted to use Mink assertions which in the basis is fine but because of the changes in how Mink sees a field (buttons are not fields) there are incompatibilities. The $value handling in the new methods were not the same either.

To make it as similar as possible I reverted the methods back to the original Simpletest versions (has to introduce constructFieldXpath() for that).

I wrapped the assertions in assertFieldByXPath() and assertNoFieldByXPath() to be able to throw the same exceptions as other assertions are.

Another thing I found out that the assertion in BrowserTestBaseTest:

$this->assertFieldById('name', 'not the value');

Was incorrect, it checked 'name' while it should have checked 'edit-name', making the test pass while it shouldn't have.

test-only should fail, the normal patch should not ;)

michielnugter’s picture

Issue summary: View changes

The last submitted patch, 7: 2868019-7-test-only.patch, failed testing.

Lendude’s picture

I quite like this approach

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -293,7 +276,7 @@ protected function assertFieldById($id, $value = '') {
   protected function assertField($field) {

@@ -306,7 +289,7 @@ protected function assertField($field) {
   protected function assertNoField($field) {

It looks like these methods don't have test coverage currently, so I think we need to add that if we are making changes to them


Nitpick:
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -564,13 +534,20 @@ protected function assertNoFieldChecked($id) {
+   * @throws ExpectationException

Needs a full path right?

dawehner’s picture

This kind-of escalated a bit, please tell me if I've been seeing this too big. All the field assertions were actually not fully compatible with the old versions. All were converted to use Mink assertions which in the basis is fine but because of the changes in how Mink sees a field (buttons are not fields) there are incompatibilities. The $value handling in the new methods were not the same either.

Just a general advice. If you realize this also tries to expand the issue title and issue summary. This helps people to figure out what is going on.

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -564,13 +534,20 @@ protected function assertNoFieldChecked($id) {
    +   * @throws ExpectationException
    

    This should use a fully qualified class name (FQCN), aka a starting slash ;)

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -637,7 +621,7 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
               }
    -          elseif ($field->getText() == $value) {
    +          elseif ($field->getTagName() !== 'input' && $field->getText() == $value) {
                 // Text area with correct text.
    

    I don't see test coverage for this change to be honest.

michielnugter’s picture

Title: Incompatible assertFieldByName and assertNoFieldByName on AssertLegacyTrait » AssertLegacyTrait field assertions not compatible with Simpletest assertions
Priority: Normal » Major
Status: Needs review » Needs work

Thanks for the reviews, I'll work on the test coverage and make it complete.

On the changing of title and summary, I did it the first time and then accidentally reloaded the page. Did update the IS the second time but forgot the title, sloppy me..

Upping to Major because it is blocking a number of conversions.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
11.09 KB

Right, quicker than expected.

10.1: Added test coverage for these assertions
10.2 & 11.1: Added the namespace
11.2: Added extra test coverage for this one. It was already indirectly covered by $this->assertNoFieldById('edit-name', NULL); because this assertion would fail without the fix but this does seem better and more complete.

michielnugter’s picture

Sorry, code reshuffle not completely succesfull, new patch and diff.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

All changed assertions now have coverage and this matches nicely with the #2807237: PHPUnit initiative directive of "Improve AssertLegacyTrait for maximum compatibility with SimpleTest."

dawehner’s picture

+1

michielnugter’s picture

Status: Reviewed & tested by the community » Needs work

Sorry about this, needed to set it back because it's still not 100% compatible as I just found out. The assertFieldsByValue() method doesn't work for checkbox fields yet.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
6.66 KB
13.2 KB

Had to rewrite some parts for the checkboxes but it's working on all tests.

Change:
- Changed the assertNoFieldByXpath to use the assertFieldValues() method to make it behave the exact same way.
- Added support for a checkbox field
- No custom exceptions where not needed anymore (previous addition in this patch). The exceptions differ but as they're normally not that relevant I don't see problems with it. Negative assertions in the test did need to catch multiple types of exceptions though.
- More testcoverage, all field methods are now covered in both positive and negative situations.

Tested with the comment conversion and the latest additions make another test work again.

dawehner’s picture

interdiff-14-18.txt 6.66 KB
The interdiff of evilness!!11!

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -585,25 +555,37 @@ protected function assertFieldByXPath($xpath, $value = NULL, $message = '') {
    +          throw new ExpectationException(new FormattableMarkup('The field resulting from :xpath was found with the provided value :value.', [
    ...
    +        throw new ExpectationException(new FormattableMarkup('The field resulting from :xpath was found.', [
    

    Let's not sure FormattableMarkup, but rather just sprintf. There is no reason to render this message as HTML output, IMHO

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -629,18 +611,22 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
    +            $found = $field->isChecked() == $value;
    ...
    +          elseif ($field->getAttribute('value') == $value) {
    

    I'm wondering in general whether === is more what we want here. Any thoughts?

  3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -773,6 +759,25 @@ protected function buildXPathQuery($xpath, array $args = []) {
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->getSession()->getPage()->findField() instead.
    ...
    +  protected function constructFieldXpath($attribute, $value) {
    +    $xpath = '//textarea[@' . $attribute . '=:value]|//input[@' . $attribute . '=:value]|//select[@' . $attribute . '=:value]';
    +    return $this->buildXPathQuery($xpath, [':value' => $value]);
    +  }
    

    It is a bit weird, could this internally use findField already? I'm also wondering whether adding the @trigger_error here would make sense, at least for now everything would cause errors, wouldn't it?

  4. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -165,6 +165,64 @@ public function testLegacyFieldAsserts() {
    +    }
    +
    +    $this->assertFieldByName('checkbox_enabled', TRUE);
    +    $this->assertFieldByName('checkbox_disabled', FALSE);
    +
    +    $this->assertNoFieldByName('checkbox_enabled', FALSE);
    +    $this->assertNoFieldByName('checkbox_disabled', TRUE);
    +
    +    try {
    +      $this->assertFieldByName('checkbox_enabled', FALSE);
    +      $this->fail('The checked "checkbox_enableRd" field was not found.');
    +
    +    }
    +    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
    +      $this->pass('The checked "checkbox_enabled" field was found.');
    +    }
    +
    +    try {
    +      $this->assertNoFieldByName('checkbox_enabled', TRUE);
    +      $this->fail('The checked "checkbox_enabled" field was not found.');
    +    }
    +    catch (ExpectationException $e) {
    +      $this->pass('The checked "checkbox_enabled" field was found.');
    +    }
    +    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
    +      $this->pass('The checked "checkbox_enabled" field was found.');
    +    }
    

    This is some serious test coverage over here

michielnugter’s picture

Status: Needs review » Needs work

Thanks for the feedback, hope to be able to post a patch today.

Interdiff actually wasn't garbage, I just changed a lot ;)

19.1: Copy paste, agree though and will change it
19.2: Yup, missed that one, will change it to ===
19.3: I think we need to update the deprecation message. findField is not compatible with the old method because it doesn't find buttons. I'll expand it with the correct method for finding buttons.
19.4: Couldn't make it complete with less. I do think it's 100% now :)

michielnugter’s picture

Fixed everything in the patch. I updated all the deprecation warnings to correctly refer to the button methods as well as the alternative is either the field or button method.

dawehner’s picture

Thank you @michielnugter
I think we should be good here ...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This work is readdy for me

michielnugter’s picture

Issue tags: +phpunit initiative

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2868019-21.patch, failed testing.

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed e918af6 to 8.4.x and 3ef5c84 to 8.3.x. Thanks!

diff --git a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
index 4f7609b..a74e4bb 100644
--- a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\FunctionalTests;
 
-use Behat\Mink\Exception\ElementNotFoundException;
 use Behat\Mink\Exception\ExpectationException;
 use Behat\Mink\Selector\Xpath\Escaper;
 use Drupal\Component\Render\FormattableMarkup;

Unused use fixed.

diff --git a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
index ee3384f..4f7609b 100644
--- a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -248,7 +248,7 @@ protected function assertNoFieldByName($name, $value = '') {
   }
 
   /**
-   * Asserts that a field exists with the given ID and val.ue.
+   * Asserts that a field exists with the given ID and value.
    *
    * @param string $id
    *   ID of field to assert.

Fixed typo on commit.

  • alexpott committed e918af6 on 8.4.x
    Issue #2868019 by michielnugter, dawehner, Lendude: AssertLegacyTrait...

  • alexpott committed 3ef5c84 on 8.3.x
    Issue #2868019 by michielnugter, dawehner, Lendude: AssertLegacyTrait...
michielnugter’s picture

Thanks @alexpott for getting this one in! This will save us a lot of work I think on the rest of the conversions.

jonathan1055’s picture

Issue summary: View changes

Thanks for fixing this, but it may cause some existing passing tests to fail as I found out yesterday. For other contrib maintainers who did not see this coming and suddenly found their tests failing when previously they passed, I am sharing what I have discovered about the change committed on 30th April.

When the field is a checkbox and you previously did not care what value the field had, just that existed, in assertFieldById() a second parameter value of empty string'' would pass the tests, but that now fails. You need to specify NULL for the $value instead. The same applies to assertFieldByName(), assertNoFieldById() and assertNoFieldByName(). See #2874410: Tests with assertFieldById fail to find field at 8.3 and 8.4, ok at 8.2 for more.

I have added this example in the summary to help other find this issue.

michielnugter’s picture

Issue tags: +Needs change record

Right, good point @jonathan1055, didn't really saw this one coming.

I think we should add a change record.

michielnugter’s picture

Draft posted:

https://www.drupal.org/node/2874609

Can someone review and publish if it's OK?

  • alexpott committed 00e2631 on 8.4.x
    Revert "Issue #2868019 by michielnugter, dawehner, Lendude:...
alexpott’s picture

Status: Fixed » Needs work

Given #32 I've reverted this change we shouldn't be breaking tests in a patch release. @jonathan1055 thank you for the report and sorry for the extra work.

alexpott’s picture

michielnugter’s picture

I would like to fix the issue with as little deviation from the original methods as possible.

The main problem I think is that the value is now tested stricter than before for checkboxes. Do you agree on this @jonathan1055 or are there more problems that you can see?

This change could work.

Instead of

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -629,18 +612,22 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
+          if ($field->getAttribute('type') == 'checkbox') {
+            // Checkbox with the correct checked state.
+            $found = $field->isChecked() === $value;

We could do:

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -629,18 +612,22 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
+          if ($field->getAttribute('type') == 'checkbox') {
+            // Checkbox with the correct checked state.
+            $found = $field->isChecked() === $value || !is_bool($value);

This would ignore the checkbox value test for checkboxes if not passed specifically as a boolean while keeping the behavior intact for non-checkboxes.
The is_bool check really needs to go inside the if because otherwise the following code would be triggered and still cause a fail:

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -629,18 +612,22 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
+          elseif ($field->getAttribute('value') == $value) {
+            // Input element with correct value.
+            $found = TRUE;

Will try to make a patch of it soon.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
14.45 KB

Don't be lazy.. Now with patch :)

Includes the on-commit fixes.

michielnugter’s picture

As @alexpott suggested on IRC, needs additional testing on the checkboxes

dawehner’s picture

Even conceptually a boolean is kinda what you expect, right?

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -248,7 +247,7 @@
   /**
-   * Asserts that a field exists with the given ID and val.ue.
+   * Asserts that a field exists with the given ID and value.
    *
    * @param string $id
    *   ID of field to assert.
@@ -613,8 +612,9 @@

@@ -613,8 +612,9 @@
       if ($fields) {
         foreach ($fields as $field) {
           if ($field->getAttribute('type') == 'checkbox') {
-            // Checkbox with the correct checked state.
-            $found = $field->isChecked() === $value;
+            // Checkbox with the correct checked state if the value was passed
+            // as a boolean.
+            $found = !is_bool($value) || $field->isChecked() === $value;

We should have some kind of test coverage for that.

jonathan1055’s picture

@alexpott from #36

@jonathan1055 thank you for the report and sorry for the extra work.

It was not too much work because I had enabled daily testing on my module at 8.2, 8.3 and 8.4 and noticed immediately when it began to fail, so I only had one day of git log to search to find this issue. :-)

we shouldn't be breaking tests in a patch release

True, but the tests which now break were only passing by fluke, i.e. they were coded incorrectly with '' as second parameter, but did not fail due to an incorrect == check. The documentation for AssertContentTrait::assertFieldById clearly states that NULL should be used for ignore the value so it was our fault that the test suddenly failed.

@michielnugter in #38

The main problem I think is that the value is now tested stricter than before for checkboxes. Do you agree on this @jonathan1055

Yes I agree.

+            $found = $field->isChecked() === $value || !is_bool($value);
This would ignore the checkbox value test for checkboxes if not passed specifically as a boolean while keeping the behavior intact for non-checkboxes.

This might be making it worse and more tests fail. I have just verified this existing behavior: a checkbox that is checked is found and passes with second parameter as NULL, TRUE, '1' or 1. A checkbox that is not checked is found with second param NULL, FALSE, '' or 0 (but fails with '0').

It is my understanding that the AssertLegacyTrait has been introduced to make it easier to switch from WebTestBase to BrowserTestBase in D8. These old assertions are deprecated and will be removed in D9 so I suggest that the code for finding checkboxes is not treated as a special case (even though it is currently a loose test) so that test results will remain as now.

So I think I have argued myself round to the point that alexpott made, that we should not be breaking existing test results, particularly for those who have already made the conversion to BTB. However, the rest of the fixes in patch should still be committed, to allow progress on conversion.

michielnugter’s picture

I've done some more research into the compatibility and found the following:

          if (isset($field['value']) && $field['value'] == $value) {
            // Input element with correct value.
            $found = TRUE;
          }

This is the Simpletests' first if statement in assertFieldsByValue.

So actually my improvement to make the checkbox assertion actually work, make it break BC: go figure :)

New patch reverted the patch to make the checkbox work correctly. I kept 1 change in the assertFieldsByValue because it fixes another bug I would like to get in. It incorrectly checked the text for all elements, including input fields. They don't have text and it made certain assertions fail.

Might still need test coverage added but I'm not sure yet. Posting the patch now to have something.

Status: Needs review » Needs work

The last submitted patch, 43: 2868019-43.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
874 bytes
13.43 KB

Test fail makes sense, it tested the correct checkbox behavior. I dropped some of the tests parts to make it pass.

Also removed an accidental R.

jonathan1055’s picture

The two tests which caused the fails in #43 are perfectly valid, and should remain. Therefore there could be something not quite right in the changes being made to AssertLegacyTrait.php. $this->assertFieldByName('checkbox_disabled', FALSE); is a valid test, and should pass with the existing code and any future change. Likewise $this->assertNoFieldByName('checkbox_disabled', TRUE); is valid and should pass. I have tested these with the existing AssertLegacyTrait.php code and they both pass.

I think that before any code is changed in AssertLegacyTrait.php we need to improve the test coverage. That way we can ensure we do not inadvertantly introduce changes which cause existing contrib module tests to fail, such as happened in #29 and #30 and which had to get reverted in #35. Or if we do make changes which cause existing tests to fail then we will know about them in advance, and make decisions about how to control the impact. I know that some of the current assertion code is a bit loose, so we will not be able to write strict tests, but mostly they do work with the correct parameters. There are also the tricky cases where the current assertions pass even when they should not (such as the example I found in #32. Not sure how we should deal with that yet.

Here is a patch which adds coverage for assertNoField(), assertField(), assertNoFieldById(), assertFieldById(), assertNoFieldByName(), assertFieldByName(), assertFieldChecked(), assertNoFieldChecked() and the specific tests on checkbox fields.

jonathan1055’s picture

FileSize
9.37 KB

Small updates from #46:

  • Split testLegacyFieldAsserts() into three smaller more managable functions, as it naturally divided into testLegacyXpathAsserts(), testLegacyFieldAssertsWithTextfields() and testLegacyFieldAssertsWithNonTextfields()
  • Added more assertions to the 'checkbox' section, as that is one area we need to be more careful about
  • Made code pass phpcs and phpcsp standards
michielnugter’s picture

Issue summary: View changes
Status: Needs review » Postponed
Related issues: +#2876357: Incomplete test coverage on AssertLegacyTrait field assertions

Great idea to add proper test-coverage first, should have started with that before even thinking about tinkering with these assertions :)

I have split it off into a separate issue to keep this issue focussed on fixing the assertions.

Issue: #2876357: Incomplete test coverage on AssertLegacyTrait field assertions

I have added some more testcoverage and will post the follow-up patch soon.

Postponing this issue for now.

michielnugter’s picture

Status: Postponed » Needs work

And the test coverage is in! Let's get this to work.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
8.13 KB
11.12 KB

I updated the assertions and added even more test coverage to cover the issue #2874410: Tests with assertFieldById fail to find field at 8.3 and 8.4, ok at 8.2. Based it on #45 as the next ones were test-coverage only.

Interdiff is hard to read because of the test coverage getting in.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

We now have passing test coverage for what caused this to be reverted, back to RTBC

jonathan1055’s picture

Status: Reviewed & tested by the community » Needs review

Great work michielnugter and Lendude, on this issue and the test coverage one.

I have been away for a while so only just catching up. Putting this back to 'Needs Review' as I am not totally sure of the statement "We now have passing test coverage for what caused this to be reverted" and I want to do a full check/review before this is RTBC. Sorry to hold it up.

Jonathan

jonathan1055’s picture

Status: Needs review » Needs work

Generally, this looks very good. Here are a few points of review:

  1. // Test with an empty string value for a checkbox, the value should be
    // ignored.
    $this->assertFieldByName('checkbox_enabled', '');
    $this->assertFieldByName('checkbox_disabled', '');
    

    Thanks for adding this. It shows that the previously sloppy passing of tests is not tightened by your changes, which is exactly what we wanted (as that made the existing contrib tests fail and caused the commit to be reverted). For completeness, though, we should also add the same two assertions for $this->assertFieldById() as that is actually the assertion which I had the problem with.

  2. Compare
    protected function assertField($field) {
      $this->assertFieldByXPath($this->constructFieldXpath('name', $field) . '|' . $this->constructFieldXpath('id', $field));
    }
    

    with

    protected function assertNoField($field) {
      $this->assertNoFieldByXPath($this->constructFieldXpath('name', $field) . '|' . $this->constructFieldXpath('id', $field), NULL);
    }
    

    For clarity, either use NULL for the second parameter of both calls, or do not use any second parameter in either. It looks like there is a purpose in having them different and that just causes developers to wonder why, and waste time investigating.

  3. In the patch in #45 you added tests to verify the behavior with duplicate buttons, as this is specifically what your original problem was all about.
    // Test that multiple fields with the same name are validated correctly.
    $this->assertFieldByName('duplicate_button', 'Duplicate button 1');
    $this->assertFieldByName('duplicate_button', 'Duplicate button 2');
    $this->assertNoFieldByName('duplicate_button', 'Rabbit');
    try {
      $this->assertNoFieldByName('duplicate_button', 'Duplicate button 2');
      $this->fail('The "duplicate_button" field with the value Duplicate button 2 was not found.');
    }
    catch (ExpectationException $e) {
      $this->pass('The "duplicate_button" field with the value Duplicate button 2 was found.');
    }
    

    These tests are not in the patch in #50 but it would be good to have them. At first do it as a separate patch just for the tests, to show it fails, then passes when the code fixes are added.

  4. In assertNoField($field) the comment does make gramatical sense and needs correcting.
    * Asserts that a field exists with the given name or ID does NOT exist.
    

    This was already wrong, not your mistake, but now is a good time to fix it.

That's enough for now. Excellent work so far.

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
13.27 KB

Thanks for the feedback! Fixes for #53

michielnugter’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all the review points have been addressed, setting to RTBC

jonathan1055’s picture

I am just doing some manual testing on this - I'll report back soon.

jonathan1055’s picture

I thought it would be useful, for clarity, to see what happens when the amended tests were run on the existing AssertLegacyTrait.php. I was expecting to see just the one failure for the duplicate buttons. However, what I found was that our additional checks for legacy behaviour with an empty string value only passed for disabled checkboxes. They fail when the checkbox is enabled. This is good becase it means the old sloppy behaviour was only half as bad as first thought ;-)

I also noticed that the new tests for multiple buttons had been added to the textfield tests when they should have been in the non-textfield tests. To simplify the testing, I have divided the 'non-textfield' tests into separate tests for Options, Button and Checkbox types. This makes each test shorter and shows more easily where the failures are. I have also (temporarily) divided the checkbox test into two, so that we can see the second failiure. As I recall on d.o. the test stops at first fail.

The attached patch is expected to fail in three tests - LegacyFieldAssertsWithButton, LegacyFieldAssertsWithCheckbox and LegacyFieldAssertsWithCheckboxTempPartTwo

For accuracy with the existing behaviour we might consider not adding the tests for enabled checkbox with empty string, even though these will pass now (with the updated AssertLegacyTrait code).

Status: Needs review » Needs work

The last submitted patch, 57: 2868019-56.tests-only.patch, failed testing. View results

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

That's good, it matched what I had observed in local testing. Here's a patch with just changes to the tests which should pass. It does not include the check for duplicate button or the change in the test form, just additional legacy tests using the exitsing AssertLegacyTrait.php and these must all remain passing when the real changes are made.

jonathan1055’s picture

OK, this is the real complete patch for final review, and interdiff between 54 (Lendude's last patch) and 60. The form and legacyTrait files are not changed since 54. The interdiff is a little hard to read, so here is a summary:

  1. testLegacyFieldAssertsWithNonTextfields() is now split in to three tests - testLegacyFieldAssertsForOptions(), testLegacyFieldAssertsForButton() and testLegacyFieldAssertsForCheckbox()
  2. To match this naming, testLegacyFieldAssertsWithTextfields() has been renamed testLegacyFieldAssertsForTextfields()
  3. The assertions for duplicate buttons have been moved from the Textfield test into the Button test
  4. New assertions added for checkbox with no second parameter passed
  5. Legacy assertions for enabled checkboxes need '1' not ''
  6. Grouped the checkbox assertions into Part 1 for name, part 2 for ID and part 3 for specifc checked state. This only required the moving of one try/catch block

This should be it!

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Refactoring makes sense, lets get this in.

jonathan1055’s picture

Status: Reviewed & tested by the community » Needs review

Yes +1 for RTBC

jonathan1055’s picture

Status: Needs review » Reviewed & tested by the community

ArgH! The status was not meant to change!. That often happens, sorry. The latest comment is displayed after I refresh the browser page, but for some reason the issue status in the form does not change to match the latest status. So when I added a comment it had the effect of reverting the status.

michielnugter’s picture

Yes +1 for RTBC and thanks @jonathan1055 for beeing thorough and taking this much time for the issue!

jonathan1055’s picture

We had quite a few eyes on the original changes, which got committed fairly promptly because the faults were hindering simpletest conversions. The code was reverted and we have now worked very thoroughly to ensure backwards compatibility and the three of us are all happy with RTBC.

Fundamentally the change is the same as before so there should not be too much (if any) resistance? What is the next step to get this committed?

Lendude’s picture

Assigned: michielnugter » Unassigned
Issue tags: -Needs change record

What is the next step to get this committed?

A committer needs to find time to look at this. So now we wait, nothing else we need to do.

We have CR so took the tag of.

jonathan1055’s picture

OK.

On the CR I have deleted the second example. That was the unintended and unwanted change which is now reverted out. The CR just covers the primary reason for creating this issue in the first place.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: 2868019-60.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

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.

larowlan’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -629,7 +611,15 @@ protected function assertFieldsByValue($fields, $value = NULL, $message = '') {
    +          if ($field->getAttribute('type') == 'checkbox') {
    ...
    +              $found = $field->isChecked() == $value;
    

    nit > should these use === because we know the type?

  2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -407,7 +413,27 @@ public function testLegacyFieldAssertsWithNonTextfields() {
    +    $this->assertNoFieldByName('duplicate_button', 'Rabbit');
    

    bonus points for more Rabbit

  • larowlan committed 4c3c1f1 on 8.5.x
    Issue #2868019 by michielnugter, jonathan1055, Lendude, dawehner,...

  • larowlan committed 1c5fff9 on 8.4.x
    Issue #2868019 by michielnugter, jonathan1055, Lendude, dawehner,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 4c3c1f1 and pushed to 8.5.x.

Cherry picked as 1c5fff9 and pushed to 8.4.x

jonathan1055’s picture

Thanks larowlan, it's good to have this in.

Just to be clear for others, you say 'cherry picked' but I have just checked and verified that the changes are identical in 8.5 and 8.4.

michielnugter’s picture

I hope this will help with decreasing the amount of work for the rest of the conversions.

Thanks for the commit and helping me achieve the most important goal this year: getting my own animal into core! This could be the first of many Rabbits to come :)

Lendude’s picture

We broke textarea's that contain \n, see #2867154-33: Form: Convert system functional tests to phpunit, not sure if this needs a revert or if we can handle this in a follow up.

  • larowlan committed b8a6b50 on 8.4.x
    Revert "Issue #2868019 by michielnugter, jonathan1055, Lendude, dawehner...

  • larowlan committed 85eefe2 on 8.5.x
    Revert "Issue #2868019 by michielnugter, jonathan1055, Lendude, dawehner...
larowlan’s picture

Status: Fixed » Needs work
Lendude’s picture

Here we go, the should_fail is the original patch with the additional test coverage.

Credit should go to @vaplas for finding and figuring this out.

Lendude’s picture

FileSize
2.34 KB

interdiff was acting a bit wonky, but here we go.

jonathan1055’s picture

Status: Needs review » Needs work

Yes, I downloaded and compared the patch with #60 to see what you had done.

This all shows that test coverage for the legacy assertions is not comprehensive, but then we knew that anyway right at the start of this issue and the previous ones. But at least now that more tests are being converted the coverage will improve :-)

jonathan1055’s picture

Status: Needs work » Needs review

Did not intend to change the status. Something is funny with form values. Back to 'needs review'

The last submitted patch, 81: 2868019-81-SHOULD_FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 81: 2868019-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Status: Needs work » Needs review
FileSize
777 bytes
17.46 KB

Ok that didn't work for elements with no value. Lets see what this does.

jonathan1055’s picture

Yes, better to limit the change just to fix just the error we have discovered. Looks fine.
Nice to see that plenty of other modules are now giving us test coverage :-)

You may also want to fix the coding standard in core/modules/system/tests/modules/test_page_test/src/Form/TestForm.php
line 79 Expected 1 space after "="; 0 found

Lendude’s picture

@jonathan1055 thanks for the feedback!

nit fixed.

vaplas’s picture

Sorry that I did not take care of this out (time is playing against me today). Thanks @Lendude, for operational work on it!

jonathan1055’s picture

Thanks for the coding standards correction. Now that core is showing these messages it is good to keep the code clean, especially with #2571965: [meta] Fix coding standards in core progressing well.

To make certain that this fix to AssertLegacyTrait does not cause any other problems with #2867154: Form: Convert system functional tests to phpunit I have tested it with the patch from #31 of that issue. All functional tests pass locally on 8.5, so here is a patch to check on d.o. Obviously this is just for testing - the patch in #89 above is the one we want to commit ;-)

jonathan1055’s picture

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

Now we know this fix does not cause any other problems, this can be RTBC again.

Lendude’s picture

@jonathan1055 thanks for the extra testing

Reupping the patch that is RTBC to make sure the last patch in the issue is the one that needs to be committed.

jonathan1055’s picture

Reupping the patch that is RTBC to make sure the last patch in the issue is the one that needs to be committed.

Is that the best practice that needs to be done? I will note that for future - I suppose it could cause confusion for a core committer who has not necessarily been involved in every twist and turn of the issue. If so, does the filename need to be changed to match the latest comment number?

Lendude’s picture

@jonathan1055 yes, the testbot does a daily run on the last patch in any RTBC'd issue, so this is to make sure it is run on the right patch. And yes it also helps the committers figure things out quicker which is patch is RTBC which is also nice.

I never change the file name just as an indication that its just a reup and people shouldn't expect an interdiff or anything, but that might just be me.

jonathan1055’s picture

Excellent. Thanks for the explanation, I should have known that.
Yes, I see it is better not to change the filename if the content of the patch has not changed.

  • larowlan committed 3609947 on 8.5.x
    Issue #2868019 by michielnugter, Lendude, jonathan1055, dawehner,...

  • larowlan committed d9b8afa on 8.4.x
    Issue #2868019 by michielnugter, Lendude, jonathan1055, dawehner,...
larowlan’s picture

Committed as 3609947 and pushed to 8.5.x

Cherry-picked as d9b8afa and pushed to 8.4.x

Thanks for the do-over.

Appreciate the combined patch to @jonathan1055, heading to review that next

larowlan’s picture

Status: Reviewed & tested by the community » Fixed
jonathan1055’s picture

Thank you again, larowlan. Sorry we caused you work to revert the commit.

Now that #2867154: Form: Convert system functional tests to phpunit is also in, I think if anyone finds a new discrepancy between Simpletest and AssertLegacyTrait during the many conversions that are happening, it should be raised as a new issue, with a comment added to this thread, so that Lendude and I are notified and can work on the fix (we know this part of the code fairly well by now).

Status: Fixed » Closed (fixed)

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