The WebTestBase provides a family of assertion-methods capable of examining form fields and their values. Regrettably the method signatures are not too consistent and that is probably the reason that they are invoked with wrong parameters every one and then. Grepping through the code the following cases turn up:
core/modules/aggregator/lib/Drupal/aggregator/Tests/ImportOpmlTest.php:
$this->assertField('refresh', '', 'Found Refresh field.');
core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php:
$this->assertField('edit-mappings-xx-browser-langcode', 'xx', 'Browser language code found.');
$this->assertField('edit-mappings-xx-browser-langcode', 'xx', 'Browser language code found.');
$this->assertField('edit-mappings-xx-drupal-langcode', 'en', 'Drupal language code found.');
$this->assertField('edit-mappings-xx-drupal-langcode', 'zh-hans', 'Drupal language code found.');
$this->assertField('edit-mappings-zh-cn-browser-langcode', 'zh-cn', 'Chinese browser language code found.');
$this->assertField('edit-mappings-zh-cn-drupal-langcode', 'zh-hans-cn', 'Chinese Drupal language code found.');
assertField has no $value parameter. Name should be first and the message second.
core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php:
$this->assertNoFieldByName('name', 'Form element was hidden.');
$this->assertNoFieldByName('name', 'Form element was hidden.');
core/modules/user/lib/Drupal/user/Tests/UserLanguageCreationTest.php:
$this->assertNoFieldByName('language[fr]', 'Language selector is not accessible.');
$this->assertNoFieldByName($relationship_name, 'Make sure that no relationship option is available');
assertNoFieldByName has $value parameter. Name should be first, value second and the message third.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2171939_21.patch | 6.61 KB | mile23 |
| #18 | 2171939-14.patch | 6.61 KB | mile23 |
| #14 | 2171939-14.patch | 6.61 KB | rpayanm |
Comments
Comment #1
sunWell spotted!
Attached patch fixes the offending assertions.
For D8, I wonder whether we shouldn't change the signature of assertField() and assertNoField() to also have a second $value argument - like all of the other assertField* methods - and simply ignore the passed $value? → Consistency appears to be more important here than an unused parameter?
Comment #3
sunWorst possible result: The tests do not pass with the corrected assertions.
I think that makes this issue at least major, if not even critical.
Comment #4
znerol commentedComment #5
znerol commentedThe last failing test was introduced with #642702: Form validation handlers cannot alter $form structure, commit b60848. I'm not so sure whether it was the idea that form-alterations would survive multiple rebuilds. Therefore I propose to fix the test and do not touch the implementation.
Comment #7
mr.baileyscore/modules/views/lib/Drupal/views/Tests/Handler/HandlerTest.php:274:
I *think* this is the correct approach and the validation handler is responsible for setting access/hiding the element on successive form builds. Would be great to get a Form API Guru to confirm this though.
Comment #8
mr.baileysChanged the incorrect invocation of assertNoFieldByName() in core/modules/views/lib/Drupal/views/Tests/Handler/HandlerTest.php
Comment #9
sunNote that #2105617: False pass with WebTestBase::assertFieldByName with select element just landed, which seems to have fixed just a single of these instances, but at the same time, it also fixed some form handling logic and added test coverage for the select form handling.
Comment #10
znerol commentedThe patch from #8 still applies cleanly to head. The thing we are still missing in this issue is a decision whether the change in
core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Callbacks.phpis justifiable or not.Comment #13
dawehner.
Comment #14
rpayanmComment #16
mile23Applies cleanly to 8.1.x. Imagine that.
Will try to start up the testbot.
Comment #18
mile23Re-uploading for drupalci.
Note that this is just a re-upload of #14. No credit to me, please.
Comment #21
mile23Moving back to 8.1.x since this is a bug and is a bunch of test improvements.
Rerolling #18 which is a reroll of #14.
Comment #29
quietone commentedTriaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.
This looks like it belongs in the Simpletest project.