Since the menu unit tests weren't really working, we just removed them for now from the module in #2482655: Drupal.org test bot doesn't run our PHPUnit tests to get the tests passing again.
Of course, removing failing tests isn't really a viable plan in the long run, so we should re-add them.

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 1
Story Points: 5

CommentFileSizeAuthor
#6 2493863-6--local_tasks_test.patch2.71 KBdrunken monkey
#6 2493863-6--local_tasks_test--interdiff.txt525 bytesdrunken monkey
#5 re_add_tests_for_the-2493863-5.patch2.66 KBborisson_
#5 interdiff.txt744 bytesborisson_
#4 re_add_tests_for_the-2493863-4.patch2.45 KBborisson_
#4 interdiff.txt1.81 KBborisson_
#1 2493863-1--readd_menu_unit_tests.patch3.09 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey commented
Status: Active » Needs review
FileSize
2493863-1--readd_menu_unit_tests.patch3.09 KB

Here is the patch that would just re-add the old tests, in the propre location.
But it's of course expected to fail.

Status: Needs review » Needs work

The last submitted patch, 1: 2493863-1--readd_menu_unit_tests.patch, failed testing.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented
Issue summary: View changes
Issue tags: +beta blocker
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.81 KB
re_add_tests_for_the-2493863-4.patch2.45 KB

I deleted the LocalActionsTest as that one was empty anyway and made sure the LocalTasksTest works.

This is probably not going to work on every machine because in the ::setUp method we refer to the 'modules/search_api' module. I'll see if I can figure that out.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
interdiff.txt744 bytes
re_add_tests_for_the-2493863-5.patch2.66 KB

The code fix my comment in #4 was actually really simple. See attached patch.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
FileSize
2493863-6--local_tasks_test--interdiff.txt525 bytes
2493863-6--local_tasks_test.patch2.71 KB 
+++ b/tests/src/Unit/Menu/LocalTasksTest.php
@@ -0,0 +1,100 @@
+    return array(
+      array('entity.search_api_index.canonical'),
+      array('entity.search_api_index.edit_form'),
+      array('entity.search_api_index.processors'),
+    );

Any reason why that doesn't contain the "Fields" tab? (Worked fine for me with it, too, so adding that.)

Otherwise, looks pretty good, thanks for making it work!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

No specific reason, no. I just forgot :(

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented

No need to be sad! This patch is looking great. Good candidate for RTBC. If someone can verify this works locally we can set that to RTBC even

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented

No need to be sad! This patch is looking great. Good candidate for RTBC. If someone can verify this works locally we can set that to RTBC even

I tested it locally, as hinted, so it should be OK.
Also, I think we can trust the test bot again, both issues were fixed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Since all 3 of us think it's RTBC, I'll bite the bullet and actually change the issue status.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Reviewed & tested by the community » Fixed

Very courageous of you, I applaud your audacity!
Committed.
Thanks again!

Status: Fixed » Closed (fixed)

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