Much of MenuTest has been written a long time ago, and this is apparent in the documentation, and in the poor choice of some variable names (e.g. '$big_user' to store an administrator). Let's clean it up a bit.

CommentFileSizeAuthor
#6 2093849-6-menutest-cleanup.patch17.06 KBangel.h
#6 interdiff-2093849-1-6.txt3.42 KBangel.h
#1 2093849-1-menutest-cleanup.patch16.22 KBpfrenssen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Active » Needs review
FileSize
2093849-1-menutest-cleanup.patch16.22 KB

Here's a patch.

  • Ordered the module list alphabetically.
  • Renamed $big_user to $admin_user
  • Renamed $std_user to $authenticated_user
  • Removed unused parameter $menu in deleteCustomMenu().
  • Fixed whitespace and punctuation issues.
  • Improved documentation.
  • Rewrote some assert messages that were mistakenly referring to admin pages as 'nodes'.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 2093849-1-menutest-cleanup.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review
Issue tags: +Novice

#1: 2093849-1-menutest-cleanup.patch queued for re-testing.

angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h commented
Assigned: Unassigned » angel.h
angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h commented
Status: Needs review » Needs work
angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h commented
FileSize
interdiff-2093849-1-6.txt3.42 KB
2093849-6-menutest-cleanup.patch17.06 KB 
+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
@@ -168,11 +195,12 @@ function addCustomMenu() {
-  function deleteCustomMenu($menu) {
+  function deleteCustomMenu() {

You removed the argument but the menu object is still passed as a parameter:

// Delete custom menu.
$this->deleteCustomMenu($this->menu);

in testMenu().

Also for 3 methods the menu id is passed as parameter:

$this->doMenuTests($this->menu->id());
$this->addInvalidMenuLink($this->menu->id());
$this->verifyAccess(403, $this->menu->id());

although this is quite unnecessary - OOP :) - we can use it directly in the methods like it was done here:

function deleteCustomMenu($menu) {
  $menu_name = $this->menu->id();

ignoring the unnesessary argument.

I created a new patch with all this in mind. You can also see an interdiff.

angel.h’s picture

angel.h
Primary language English
Location Plovdiv
CreditAttribution: angel.h commented
Assigned: angel.h » Unassigned
Status: Needs work » Needs review
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented

Nice catch! The test looks much better now.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented

6: 2093849-6-menutest-cleanup.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2093849-6-menutest-cleanup.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented

Patch still applies cleanly, and locally this test is green. Retesting.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review

6: 2093849-6-menutest-cleanup.patch queued for re-testing.

parthipanramesh’s picture

parthipanramesh CreditAttribution: parthipanramesh commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Good job!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

6: 2093849-6-menutest-cleanup.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

6: 2093849-6-menutest-cleanup.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Minor

Nice work! Recategorizing as minor since it is pure cleanup for test code.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +RTBC 4+ weeks

Also, this patch was RTBCed Nov. 28 and has just been retested since.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

6: 2093849-6-menutest-cleanup.patch queued for re-testing.

webchick’s picture

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

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: -RTBC over 4 weeks