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.
Problem/Motivation
The current assertFieldById() and assertNoFieldById() both incorrectly check the field. The current implementations (indirectly) the findField() method. This causes the following problems:
- The field is searched generically by input id, name or label instead of only the ID. This leads to false positives for assertNoFieldById()
- Buttons are ignored now, causing no results on a valid ID. They were not ignored in the WebTestBase and should therefore still work in this legacy trait.
Proposed resolution
Rewrite the implementations to use the findById() methods and keep the thrown exceptions the same so we don't break BC.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2862947-35.patch | 6.49 KB | michielnugter |
#35 | interdiff-32-35.txt | 496 bytes | michielnugter |
#32 | 2862947-32.patch | 6.5 KB | jofitz |
#32 | interdiff-28-32.txt | 1.88 KB | jofitz |
#28 | interdiff.txt | 1.23 KB | boaloysius |
Comments
Comment #2
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI also had to fix 2 assertions in BrowserTestBase because they actually checked on name and not id.
Comment #3
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #4
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #5
klausiData types in docs need to be fully namespaced, see https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
we are searching by ID here, so the third parameter should be just "id", right?
FQDN again.
not super happy with that logic. we could have a first condition $field === NULL and return immediately. Because currently we are checking $field !== NULL twice.
Comment #6
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedFixed the mentioned feedback. Is the assertNoFieldById() better now?
Comment #7
klausiCool, just one minor thing:
That comment does not really fit here. Should be something like "Return early if the field could not be found as expected."
Comment #8
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedAs discussed offline, removed the comments because they explain a situation that is not apparent in the new code (it stated the change in code but the old code is obviously not there anymore).
Comment #9
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #10
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedForgot the interdiff..
Comment #11
klausiLooks good now, thanks!
Comment #12
alexpottIsn't this too liberal and will find anything with this ID - not just fields?
The old simpletest code was doing
in \Drupal\simpletest\AssertContentTrait::constructFieldXpath()
Comment #13
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented@alexpott: True, it actually returns every element with the requested ID and not only fields.
Mink is stricter than Simpletest in that buttons (input type submit) are not returned by findField().
I'll try to figure out a way to completely minic the Simpletest method.
Comment #14
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #15
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI changed the implementation to use the original xpath so now it's exactly the same as it was.
I attached a converted test with both methods as prove it works with the CachedDataUITest test.
Comment #16
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #17
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #18
klausiThanks, agreed with using xpath. Looks good!
Comment #20
alexpottThe whole behaviour with $value here is really odd - but as far as I can we're duplicating the behaviour of Simpletest fwiw.
There's quite good testing around the $value path in assertNoFieldById() in \Drupal\FunctionalTests\BrowserTestBaseTest::testLegacyFieldAsserts. However what that is missing is any coverage of assertFieldById(). I think we should add that here. Specifically I think we should test for one of the reported bugs:
Comment #21
jofitz CreditAttribution: jofitz at ComputerMinds commentedThis sounds right up my street...
Comment #22
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded a success-test and failure-test for both assertFieldById() and assertNoFieldById().
Comment #23
alexpottSo now we just need to add value testing for assertByFieldId() - we have it for the assertNoFieldById() case. And then we have full test coverage!
Comment #24
boaloysius CreditAttribution: boaloysius as a volunteer commentedI was trying my hand in adding test for assertFieldById. I think similar code like the following assertNoFieldById code with assertFieldById will do.
But I am unable to access the URL example.com/test-field-xpath over the browser. It is giving page not found.
How to access that path over browser so that I can inspect and find the id and value in the page.
I also tried to access all the drupalGet paths in the BrowserTestBaseTest file, but failed.
Thank you. I am new in writing code for BrowserTest.
Comment #25
alexpott@boaloysius to get to that path you need to enable the
test_page_test
module. Since it is a test module you need to add$settings['extension_discovery_scan_tests'] = TRUE;
to your settings.php. To see what modules a test has enabled you need to look in the test class's $module property - normally near the top of the file. And also the classes it inherits from. (in this instance there aren't any in the parent classes). So this test haspublic static $modules = ['test_page_test', 'form_test', 'system_test'];
.Comment #26
boaloysius CreditAttribution: boaloysius as a volunteer commentedThank you @alexpott.
Comment #27
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #28
boaloysius CreditAttribution: boaloysius as a volunteer commentedCurrently in this patch we are testing for improper no-value and NULL. Should we add test for wrong value and wrong id?
Comment #29
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #31
boaloysius CreditAttribution: boaloysius as a volunteer commentedWhat is wrong with this code?
This code checks edit-name field for default $value=''. But the default value of the edit-name field is 'Test name'. So it should throw an exception and pass no?
Comment #32
jofitz CreditAttribution: jofitz at ComputerMinds commented@boaloysius Your test was (almost) perfect, the problem was with the assertFieldById() method - the default $value parameter was wrong: it neither matched the documentation nor the WTB method it is replacing.
I have corrected the parameter, adjusted the exception that needs to be caught and added another assert for good measure!
BrowserTestBaseTest now passes locally, the only question is whether this parameter change breaks any other tests...
Comment #33
boaloysius CreditAttribution: boaloysius as a volunteer commentedthank you Jo Fitzgerald
Comment #34
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedThe deprecated documentation should be updated. There is something wrong asking to use some methods we don't find in the new test
Same that previous comment. @deprecated documentation is different from fix
Remove white spaces from the end of the line.
Comment #35
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented1 and 2: These messages are still correct, only the implementation was fixed to conform to the old WebTestBase methods. They are still deprecated and should be replaced with the mentioned methods. The test doesn't contain these methods because right now we're only adding coverage for the methods added in this function. If any additional coverage is required a new issue should be opened for that.
3: Fixed the whitespace.
As it's only a whitespace change I'm RTBC'ing it. Thanks for the improved test coverage!
Comment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedRetests passed, back to RTBC.
Comment #38
alexpottCommitted 4c01abe and pushed to 8.4.x. Thanks!
Committed 88747b7 and pushed to 8.3.x. Thanks!
Committed this to 8.3.x too because it is super important that assertions behave the same way.
Comment #41
boaloysius CreditAttribution: boaloysius as a volunteer and at Google Summer of Code commented