Problem/Motivation

AssertLegacyTrait::assertOptionByText() is deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use $this->assertSession()->optionExists() instead. See https://www.drupal.org/node/3129738

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

jungle’s picture

Issue tags: +Deprecated assertions

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

bibliophileaxe’s picture

Status: Active » Needs review
FileSize
5.47 KB
jungle’s picture

FileSize
6.17 KB
1015 bytes

Thanks, @akanksha-hp!

  1. Adding a test.
  2. \Drupal\KernelTests\AssertContentTrait::assertOptionByText() exists.

    Removing it directly here is a BC change so that we can not do it here, right? File an issue to deprecate it, and adding a test?

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup
+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -460,23 +460,23 @@ public function testDatelistWidget() {
-    $this->assertOptionByText("edit-$field_name-0-value-year", t('Year'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-year", t('Year'));
     $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-month\"]", NULL, 'Month element found.');
     $this->assertOptionSelected("edit-$field_name-0-value-month", '', 'No month selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-month", t('Month'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-month", t('Month'));
     $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-day\"]", NULL, 'Day element found.');
     $this->assertOptionSelected("edit-$field_name-0-value-day", '', 'No day selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-day", t('Day'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-day", t('Day'));
     $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-hour\"]", NULL, 'Hour element found.');
     $this->assertOptionSelected("edit-$field_name-0-value-hour", '', 'No hour selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-hour", t('Hour'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-hour", t('Hour'));
     $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-minute\"]", NULL, 'Minute element found.');
     $this->assertOptionSelected("edit-$field_name-0-value-minute", '', 'No minute selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-minute", t('Minute'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-minute", t('Minute'));
     $this->assertNoFieldByXPath("//*[@id=\"edit-$field_name-0-value-second\"]", NULL, 'Second element not found.');
     $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-ampm\"]", NULL, 'AMPM element found.');
     $this->assertOptionSelected("edit-$field_name-0-value-ampm", '', 'No ampm selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-ampm", t('AM/PM'));
+    $this->assertSession()->optionExists("edit-$field_name-0-value-ampm", t('AM/PM'));

These would need to get the t() calls removed, too. But I suppose it's a follow-up.

Other than that, it's RTBC for me.

mondrake’s picture

#4.2 I do not think that needs to be deprecated - it's a separate implementation, for the kernel tests. What we are removing here are the Simpletest legacy for functional/javascript tests. Now - that does not seem to be used for kernel tests either, though... maybe a separate issue might investigate which of the \Drupal\KernelTests\AssertContentTrait methods are really needed?

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -262,6 +262,19 @@ public function testAssertHeader() {
+  /**
+   * Tests legacy assertOptionByText().
+   *
+   * @group legacy
+   * @expectedDeprecation AssertLegacyTrait::assertOptionByText() is deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use $this->assertSession()->optionExists() instead. See https://www.drupal.org/node/3129738
+   */
+  public function testAssertOptionByText() {
+    $account = $this->drupalCreateUser();
+    $this->drupalLogin($account);
+    $this->drupalGet('form-test/select');
+    $this->assertOptionByText('edit-select', 'one');
+  }
+
   /**
    * Tests legacy text asserts.
    */
@@ -480,9 +493,9 @@ public function testFieldAssertsForOptions() {

@@ -480,9 +493,9 @@ public function testFieldAssertsForOptions() {
     $this->drupalGet('test-field-xpath');
 
     // Option field type.
-    $this->assertOptionByText('options', 'one');
+    $this->assertSession()->optionExists('options', 'one');
     try {
-      $this->assertOptionByText('options', 'four');
+      $this->assertSession()->optionExists('options', 'four');
       $this->fail('The select option "four" was found.');

Actually, maybe we do not need the separate test. Just add the @expectedDeprecation to the docblock of testFieldAssertsForOptions, without replacing the calls in the method.

xjm’s picture

Title: Replace usages of AssertLegacyTrait::assertOptionByText, that is deprecated » Replace usages of deprecated AssertLegacyTrait::assertOptionByText()

Good catch on #7.

Agreed on #5 also; I think we should have a separate issue to stop translating assertion messages. There's #2552067: Remove t() from assertion messages in tests, which is very old (still using SafeMarkup::format(), of all things). That said, it might be better to do the issues to remove unnecessary assertion messages first, since that will also get rid of many of the t(), and then we can proceed with other t() using on strings in value comparisons. In any case, best not to change here.

mondrake’s picture

#5 that’s not about assertion messages, it’s the text to be found in the option. The method dropped the assertion message argument already.

xjm’s picture

@mondrake Well I did refer to both. :) Just could not find the issue for removing other unnecessary translation use. But yeah, I see now that there's no messages in the patch.

sja112’s picture

Assigned: Unassigned » sja112
sja112’s picture

Assigned: sja112 » Unassigned
Status: Needs work » Needs review
FileSize
6.2 KB
1.83 KB

Addressing #7.

Status: Needs review » Needs work

The last submitted patch, 12: 3139422-12.patch, failed testing. View results

sja112’s picture

Assigned: Unassigned » sja112
sja112’s picture

Assigned: sja112 » Unassigned
Status: Needs work » Needs review
FileSize
5.8 KB
1.77 KB

Realized the mistake I was making in #12. Corrected it in this patch.

daffie’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

All occurrences of $this->assertOptionByText() have been replaced.
Deprecation message testing has been added.
The deprecation message suppression has been removed.
Followup created. See: #3144732: Remove the calls to t() from $this->assertSession()->optionExists().
The point from @mondrake in comment #7 has been addressed.
All code changes look good to me.
For me it is RTBC.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 803a34b and pushed to 9.1.x. Thanks!

Backported the test changes and not the deprecation changes to 8.9.x and 9.0.x.

Committed and pushed 5a289071ee to 9.0.x and 69dd75d974 to 8.9.x. Thanks!

  • alexpott committed 803a34b on 9.1.x
    Issue #3139422 by sja112, jungle, akanksha-hp, mondrake, xjm: Replace...

  • alexpott committed 5a28907 on 9.0.x
    Issue #3139422 by sja112, jungle, akanksha-hp, mondrake, xjm: Replace...

  • alexpott committed 69dd75d on 8.9.x
    Issue #3139422 by sja112, jungle, akanksha-hp, mondrake, xjm: Replace...

Status: Fixed » Closed (fixed)

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