Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
16.77 KB

Work in progress, this will fail.

Status: Needs review » Needs work

The last submitted patch, 2: browsertest-link-2763013-2.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
22.85 KB
8.03 KB

Fixed fails. Copied FieldUiTestTrait and modified it to work with BrowserTestBase by replacing the assertFieldByXpath() calls.

Mile23’s picture

As of July 11, core's 8.2.x branch test has 18,617 passes, but the patch shows 18,614.

The patch still applies, so I'm running the testbot again to see if the number changes.

klausi’s picture

The number of assertions goes down because Mink does not use PHPUnit assert() methods. It has its own WebAssert class with its own assert methods, completely standalone and unrelated to PHPUnit. So there is no way for PHPUnit to count assertions in there.

Is that a concern for us? Is the total number of assertions meaningful?

dawehner’s picture

@mile23, @klausi
Also you have to keep in mind, depending on the batch processes in some of the update path / config import etc. tests the amount of assertions might vary on a test by test base anyway.

This is just some quick review. Sorry for slacking on some of the reviews recently.

  1. +++ b/core/modules/field_ui/tests/src/Functional/FieldUiTestTrait.php
    @@ -0,0 +1,127 @@
    +  public function fieldUIAddNewField($bundle_path, $field_name, $label = NULL, $field_type = 'test_field', array $storage_edit = array(), array $field_edit = array()) {
    ...
    +  public function fieldUIAddExistingField($bundle_path, $existing_storage_name, $label = NULL, array $field_edit = array()) {
    ...
    +  public function fieldUIDeleteField($bundle_path, $field_name, $label, $bundle_label) {
    

    This is also maybe a bad question: Is there a particular reason it to be public?

  2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -329,9 +329,9 @@ function testLinkTitle() {
    -    $this->renderTestEntity($id);
    -    $expected_link = \Drupal::l($title, Url::fromUri($value));
    -    $this->assertRaw($expected_link);
    +    $output = $this->renderTestEntity($id);
    +    $expected_link = (string) \Drupal::l($title, Url::fromUri($value));
    +    $this->assertContains($expected_link, $output);
       }
    

    I like the new variant. Its more explicit ...

  3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -64,6 +67,10 @@ protected function assertText($text) {
    +      // It will provide the value HTML entity decoded, so we need to do the
    +      // same here for the text to actually match.
    

    Can we clarify that the PHP DOM api does the decoding, not mink/browserkit?

claudiu.cristea’s picture

Status: Needs review » Postponed

I'm postponing this on #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3). There also other conversions needing that, so let's fix it in a dedicated issue.

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.

claudiu.cristea’s picture

Status: Postponed » Needs review
FileSize
6.76 KB
20.97 KB

Un-postponing as #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3) is in. As \Drupal\Tests\field_ui\Functional\FieldUiTestTrait is new code, it should not use deprecated methods.

Status: Needs review » Needs work

The last submitted patch, 10: 2763013-10.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
21.39 KB
716 bytes

assertNoFieldByName() was implemented in the meantime.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Dublin2016

assertFieldByXPath() is now implemented in #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests, so I think we don't need to copy FieldUiTestTrait. We should try to use the existing legacy trait.

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
14.66 KB

Re-rolled.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -272,7 +272,7 @@ function testLinkTitle() {
-        $this->assertNoFieldByName("{$field_name}[0][title]", '', 'Link text field not found.');
+        $this->assertSession()->fieldNotExists("{$field_name}[0][title]");

->assertNoFieldByName() exists now, so we should not need to convert that line. Can you revert the changes to assertNoFieldByName() in this patch and see if that works now?

Jo Fitzgerald’s picture

Status: Needs work » Needs review

The test fails if the change is reverted to assertNoFieldByName(), unless the second and third parameters are removed (I have tested this locally). I can upload the patches if required?

I suggest it is better to stick with the change in #15 rather than a change to the parameters of a deprecated function.

klausi’s picture

Assigned: klausi » Unassigned
Status: Needs review » Postponed

Aha, but that means our assertNoFieldByName() is not really compatible and we need to fix it. Opened #2857725: Improve assertNoFieldByName() compatibility for empty strings, postponing on that.

klausi’s picture

Status: Postponed » Needs work

That got in, so we can rework this patch now.

Jo Fitzgerald’s picture

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.75 KB

Re-rolled.
Re-instated reference to assertNoFieldByName() now that it has been corrected.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -329,15 +329,15 @@ public function testLinkTitle() {
    -  public function testLinkFormatter() {
    +  public function xtestLinkFormatter() {
    

    debugging change that should be reverted.

  2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -492,7 +492,7 @@ public function testLinkFormatter() {
    -  public function testLinkSeparateFormatter() {
    +  public function xtestLinkSeparateFormatter() {
    

    same here

  3. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
    @@ -617,7 +617,7 @@ public function testLinkSeparateFormatter() {
    -  public function testLinkTypeOnLinkWidget() {
    +  public function xtestLinkTypeOnLinkWidget() {
    

    and here

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
14.25 KB

I'm sorry, that was careless of me.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Confirmed that only a Views test is missing in link module, but we decided in other issues that we will convert those independently with or after #2747167: Convert first part of web tests of views_ui.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a333b55 and pushed to 8.4.x. Committed 5a5e2cc and pushed to 8.3.x. Thanks!

There was a new test in 8.4.x so on cherry-pick I just merged it correctly - the new test is testLinkTypeOnLinkWidget() and it still is not in 8.3.x.

  • alexpott committed a333b55 on 8.4.x
    Issue #2763013 by Jo Fitzgerald, claudiu.cristea, klausi: Convert web...

  • alexpott committed 5a5e2cc on 8.3.x
    Issue #2763013 by Jo Fitzgerald, claudiu.cristea, klausi: Convert web...

Status: Fixed » Closed (fixed)

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