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.
Testing the following output:
<select name="test">
<option value="foo">Foo</option>
<option value="bar" selected="selected">Bar</option>
</select>
with an assert:
$this->assertFieldByName('test', 'foo');
passes when it should fail.
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff.txt | 758 bytes | olli |
#26 | drupal-2105617-26.patch | 5.54 KB | olli |
#24 | drupal-2105617-24.patch | 6.85 KB | olli |
#24 | interdiff.txt | 1.63 KB | olli |
#18 | drupal-2105617-18.patch | 7.33 KB | olli |
Comments
Comment #1
olli CreditAttribution: olli commentedThis seems to fix it.
Comment #3
olli CreditAttribution: olli commentedThose failing tests are checking if the element exists so we should use NULL as the second parameter.
Comment #4
olli CreditAttribution: olli commentedtagging
Comment #5
dawehnerLet's add a @group Drupal at least.
HAHAHAHHA
Let's start with provider to avoid some confusion I had.
Comment #6
Mile23Yah, this is super confusing, especially since method names beginning with 'assert' tend to have special meaning as well. ("Wait, what, is that a new assertion being defined??")
Name it providerTestAssertFieldByName() please.
+1 on @group Drupal and also add @group Simpletest.
Comment #7
olli CreditAttribution: olli commentedThanks for the reviews! I like that provider prefix.
Moved the output to test to separate files and added t() which is called in WebTestBase::parse().
Comment #9
olli CreditAttribution: olli commentedThis should fix it.
Comment #10
olli CreditAttribution: olli commentedReplaced t() in parse() with String::format() instead.
Comment #11
dawehnerI guess we should name the file with .html ending.
Still mindblowing :P
Let's add a {@inheritdoc} now.
Comment #12
olli CreditAttribution: olli commentedThanks for your review!
1. changed from html/filename to filename.html
2. we also have TestBaseTest
3. done
Comment #13
dawehnerThank you, that is perfect now.
Comment #14
Xano12: drupal-2105617-12.patch queued for re-testing.
Comment #15
Xano12: drupal-2105617-12.patch queued for re-testing.
Comment #16
Berdir12: drupal-2105617-12.patch queued for re-testing.
Comment #18
olli CreditAttribution: olli commentedReroll
Comment #19
dawehnerRe-RTBC
Comment #20
webchick18: drupal-2105617-18.patch queued for re-testing.
Comment #21
alexpottWhy do we think that we can do this?
t()
is used all over WebTestBase including inassertFieldByName()
just removing one call to it seems inconsistent. I would prefer the original solution to this issue in #9.Comment #22
olli CreditAttribution: olli commentedThanks for the review!
Because of #500866: [META] remove t() from assert message. Few t()s have already been converted to String::format().
Comment #23
alexpottYes but following that link to find the issues you find #1601146: Allow custom assertion messages using predefined placeholders which has a different solution - and one that addresses all usages of t() in WebTestBase - that's why I think just removing one usage is inconsistent.
Comment #24
olli CreditAttribution: olli commentedOk!
Comment #25
olli CreditAttribution: olli commentedComment #26
olli CreditAttribution: olli commentedReroll.
Comment #27
Mile23Coding standards. Lose the period at the end.
Other than that, RTBC (again). :-)
Comment #28
alexpottCommitted 20d5b0f and pushed to 8.x. Thanks!
Fixed on commit.
Comment #29
alexpottOops an automated version of me got it wrong
Comment #30
alexpottComment #31
sunFWIW, this is/was partially a duplicate of #2171939: Invocation of WebTestBase::assertField and WebTestBase::assertNoFieldByName methods with wrong parameters — which discovered many more bogus invocations.