Problem/Motivation

#2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin had to change a test when it really should not have needed to. (See comment #70 1.)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
@@ -112,7 +112,7 @@ function manageFieldsPage($type = '') {
-    $this->assertIdentical("$url/delete", (string) $result[2]['href']);
+    $this->assertIdentical("$url/delete", (string) $result[3]['href']);

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because test is accidentally testing something it should not
Issue priority Minor (or Normal) since this is very isolated per https://www.drupal.org/core/issue-priority
Unfrozen changes Unfrozen because it only changes automated tests

So this is allowed in the beta since it is only effecting an automated test.

Proposed resolution

change the test to not explicitly depend on absolute values of indexes.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

No.

API changes

No.

CommentFileSizeAuthor
#25 2269033-25.patch1.54 KBrpayanm
#25 2269033-interdiff.txt1.48 KBrpayanm
#22 2269033-22.patch1.46 KBzaporylie
#22 interdiff-2269033-18-22.txt976 byteszaporylie
#18 2269033-18.patch1.46 KBcharginghawk
#15 2269033-15.patch1.46 KBzaporylie
#10 2269033-10.patch1.42 KBrpayanm
#8 2269033-8.patch1.42 KBswentel
#4 2269033-fix-array-order.patch1.17 KBlieb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lieb’s picture

lieb CreditAttribution: lieb commented

New contributor - looking into this.

lieb’s picture

lieb CreditAttribution: lieb commented

File in question is now in ./core/modules/field_ui/src/Tests/

lieb’s picture

lieb CreditAttribution: lieb commented
Assigned: Unassigned » lieb
lieb’s picture

lieb CreditAttribution: lieb commented
Assigned: lieb » Unassigned
Status: Active » Needs review
FileSize
2269033-fix-array-order.patch1.17 KB
lieb’s picture

lieb CreditAttribution: lieb commented

Okay, the patch passed but it is logically incorrect. With this patch it is possible for these 3 assertions to not run at all. the correct solution is to find a way to index the array on some static value. I will keep working on this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Component: plugin system » field_ui.module
lorique’s picture

lorique CreditAttribution: lorique commented
Status: Needs review » Needs work

I reviewed this, and logically it looks fine. Not sure if we're covering all the cases.

I ran tests with:
php core/scripts/run-tests.sh --class "Drupal\field_ui\Tests\ManageFieldsTest"

And got the following error:

Drupal test run
---------------

Tests to be run:
  - Drupal\field_ui\Tests\ManageFieldsTest

Test run started:
  Thursday, July 17, 2014 - 10:32

Test summary
------------


Fatal error: Call to a member function getSetting() on a non-object in /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php on line 254

Call Stack:
    0.0010     392608   1. {main}() /var/www/drupal/d8/core/scripts/run-tests.sh:0
    0.4269   14184200   2. simpletest_script_run_one_test() /var/www/drupal/d8/core/scripts/run-tests.sh:36
    0.4334   15658528   3. Drupal\simpletest\TestBase->run() /var/www/drupal/d8/core/scripts/run-tests.sh:626
   11.3195   48783680   4. Drupal\field_ui\Tests\ManageFieldsTest->testCRUDFields() /var/www/drupal/d8/core/modules/simpletest/src/TestBase.php:860
   11.3287   48616464   5. Drupal\field_ui\Tests\ManageFieldsTest->updateField() /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php:72
   11.3321   48635704   6. Drupal\field_ui\Tests\ManageFieldsTest->assertFieldSettings() /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php:162

FATAL Drupal\field_ui\Tests\ManageFieldsTest: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest808062' for test ID 3.
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP Fatal error:  Call to a member function getSetting() on a non-object in /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php on line 254
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP Stack trace:
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   1. {main}() /var/www/drupal/d8/core/scripts/run-tests.sh:0
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   2. simpletest_script_run_one_test() /var/www/drupal/d8/core/scripts/run-tests.sh:36
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   3. Drupal\simpletest\TestBase->run() /var/www/drupal/d8/core/scripts/run-tests.sh:626
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   4. Drupal\field_ui\Tests\ManageFieldsTest->testCRUDFields() /var/www/drupal/d8/core/modules/simpletest/src/TestBase.php:860
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   5. Drupal\field_ui\Tests\ManageFieldsTest->updateField() /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php:72
[17-Jul-2014 10:32:23 Europe/Copenhagen] PHP   6. Drupal\field_ui\Tests\ManageFieldsTest->assertFieldSettings() /var/www/drupal/d8/core/modules/field_ui/src/Tests/ManageFieldsTest.php:162

- Removed test site directory.
- Removed 58 leftover tables.

Test run duration: 11 sec

I don't know if this is related, but its the same file. I'm setting this issue back to needs work.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Needs review
FileSize
2269033-8.patch1.42 KB

Added an additional test because if we change the label on the links by accident, the test won't cover it.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

doing https://www.drupal.org/contributor-tasks/update-allowed-beta
to evaluate https://www.drupal.org/contribute/core/beta-changes

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2269033-10.patch1.42 KB

rerolled...

Status: Needs review » Needs work

The last submitted patch, 10: 2269033-10.patch, failed testing.

Status: Needs work » Needs review

zaporylie queued 10: 2269033-10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2269033-10.patch, failed testing.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: Unassigned » zaporylie

I will re-roll #10

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: zaporylie » Unassigned
Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
2269033-15.patch1.46 KB

Here is re-rolled patch #10, with additional break; at the and of case 'Delete instance.':

Status: Needs review » Needs work

The last submitted patch, 15: 2269033-15.patch, failed testing.

charginghawk’s picture

charginghawk CreditAttribution: charginghawk commented
Assigned: Unassigned » charginghawk
charginghawk’s picture

charginghawk CreditAttribution: charginghawk commented
Assigned: charginghawk » Unassigned
Status: Needs work » Needs review
FileSize
2269033-18.patch1.46 KB

The error is due to the fact that 'Edit field settings' was changed to 'Edit storage settings' in issue 2312093:

https://www.drupal.org/node/2312093
https://github.com/drupal/drupal/commit/1476c56c62e4d84ef3e9a57029a92b1f...

Re-rolling with that minor change.

Status: Needs review » Needs work

The last submitted patch, 18: 2269033-18.patch, failed testing.

manningpete’s picture

manningpete CreditAttribution: manningpete commented
Issue tags: -Needs reroll

Last patch applies; no reroll needed.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: Unassigned » zaporylie
zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie commented
Assigned: zaporylie » Unassigned
Status: Needs work » Needs review
Related issues: +#2312093: Rename FieldInstanceConfig to FieldConfig
FileSize
interdiff-2269033-18-22.txt976 bytes
2269033-22.patch1.46 KB

Good work @charginghawk! Thanks to you I discovered that all dropdown menu titles was changed in #2312093: Rename FieldInstanceConfig to FieldConfig. I've updated patch - should be ok right now.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Good to go

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
@@ -122,11 +122,29 @@ function manageFieldsPage($type = '') {
     $result = $this->xpath('//ul[@class = "dropbutton"]/li/a');
...
+    foreach ($result as $res) {

Rather than the variable name of $result / $res how about $operation_links / $link

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm at SeeD EM commented
Status: Needs work » Needs review
FileSize
2269033-interdiff.txt1.48 KB
2269033-25.patch1.54 KB

Here the patch.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie at Ny Media AS commented
Status: Needs review » Reviewed & tested by the community

#25 follows alexpott suggestion, nothing more, so I'm bumping it back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Looks good!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 03d3c27 on 8.0.x 
    Issue #2269033 by zaporylie, rpayanm, lieb, swentel, charginghawk,...

Status: Fixed » Closed (fixed)

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