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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2763013-23.patch | 14.25 KB | jofitz |
#23 | interdiff-21-23.txt | 1.84 KB | jofitz |
#21 | 2763013-21.patch | 15.75 KB | jofitz |
#15 | 2763013-15.patch | 14.66 KB | jofitz |
#12 | interdiff.txt | 716 bytes | claudiu.cristea |
Comments
Comment #2
klausiWork in progress, this will fail.
Comment #4
klausiFixed fails. Copied FieldUiTestTrait and modified it to work with BrowserTestBase by replacing the assertFieldByXpath() calls.
Comment #5
Mile23As 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.
Comment #6
klausiThe 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?
Comment #7
dawehner@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.
This is also maybe a bad question: Is there a particular reason it to be public?
I like the new variant. Its more explicit ...
Can we clarify that the PHP DOM api does the decoding, not mink/browserkit?
Comment #8
claudiu.cristeaI'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.
Comment #10
claudiu.cristeaUn-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.Comment #12
claudiu.cristeaassertNoFieldByName() was implemented in the meantime.
Comment #13
klausiassertFieldByXPath() 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.
Comment #15
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #16
klausi->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?
Comment #17
jofitz CreditAttribution: jofitz at ComputerMinds commentedThe 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.
Comment #18
klausiAha, 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.
Comment #19
klausiThat got in, so we can rework this patch now.
Comment #20
jofitz CreditAttribution: jofitz at ComputerMinds commentedRequires re-roll due to #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase.
Comment #21
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Re-instated reference to assertNoFieldByName() now that it has been corrected.
Comment #22
klausidebugging change that should be reverted.
same here
and here
Comment #23
jofitz CreditAttribution: jofitz at ComputerMinds commentedI'm sorry, that was careless of me.
Comment #24
klausiLooks 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.
Comment #25
alexpottCommitted 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.