Coming from #2513626: [Regression] Module permission links missing from module list page, there is test coverage for the administrative module listing page to ensure that the "Configure" and "Permissions" links show up correctly for each module, but there is no test to check that the "Help" link does.

So we need a patch to add a test for the "Help" link. That will entail a couple of things:

1) Adding a hook_help() implementation to system_test.module;
2) Adding a test for "Help" in ModulesListFormWebTest.

CommentFileSizeAuthor
#15 interdiff-15.txt1.32 KBjoshi.rohit100
#15 test-for-module-help-link-2516690-15.patch1.87 KBjoshi.rohit100
#5 test-for-module-help-link-2516690-5.patch1.64 KBmlevasseur
#8 interdiff-2516690-5-8.txt1.41 KBmlevasseur
#8 test-for-module-help-link-2516690-8.patch1.91 KBmlevasseur
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jgeryk’s picture

jgeryk CreditAttribution: jgeryk commented

I'll try to work on this

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

@jgeryk - If you are working on this, please assign it to yourself.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes
mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented

@jgeryk - Are you still working on this? If not I'll assign it to myself.

mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented
Assigned: Unassigned » mlevasseur
Status: Active » Needs review
FileSize
test-for-module-help-link-2516690-5.patch1.64 KB

I added the hook_help() function to system_test.module and a test to see if the module list form contains the link, but the test fails. So I'm thinking I either

a) Wrote it incorrectly...?
b) Need to add some code somewhere else that I'm not aware of.

I modelled the hook_help() function off of http://www.drupalcontrib.org/api/drupal/drupal%21core%21modules%21action... to make sure I was doing it correctly. What's missing?

Status: Needs review » Needs work

The last submitted patch, 5: test-for-module-help-link-2516690-5.patch, failed testing.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim at TEN7 commented

Thanks for picking this up! The assertion looks right, but the test is failing because Help module isn't in the list of modules to enable when the test is set up. There's an array of modules to enable at the top of ModulesListFormWebTest; try it again with 'help' in there.

+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -124,3 +124,16 @@ function system_test_module_preinstall($module) {
+/**
+ * Implements hook_help().
+ */
+function system_test_help($path, $arg) {

Yikes, drupalcontrib.org is really out-of-date. The hook_help() implementation hasn't had that signature in at least a year. Instead, look at the hook_help() example in Help module's help.api.php file. Fixing system_test_help() isn't strictly necessary for this particular test to pass, but we should get it right anyway.

mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented
Status: Needs work » Needs review
FileSize
test-for-module-help-link-2516690-8.patch1.91 KB
interdiff-2516690-5-8.txt1.41 KB

Thanks for the link! I was looking around trying to find proper hook_help() usages, although I suppose I could've just checked out block.module or similar to see how to do it.

The tests pass properly now, and I updated the hook_help() to the standard usage.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs review » Needs work
Issue tags: -Needs tests 
+++ b/core/modules/system/tests/modules/system_test/system_test.module
@@ -124,3 +124,16 @@ function system_test_module_preinstall($module) {
+function system_test_help($route_name, RouteMatchInterface $route_match) {

There must be a space after the switch statement in the hook_help().

Also, it is no big deal as this is a test module, but hook_help implementations are usually toward the top of a module file.

The RouteMatchInterface needs a `use` statement.

mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented
Assigned: mlevasseur » Unassigned

I'm un-assigning myself from this for the time being, will pick it back up on Monday if it's still unfinished by then.

The last submitted patch, 8: test-for-module-help-link-2516690-8.patch, failed testing.

Les Lim queued 8: test-for-module-help-link-2516690-8.patch for re-testing.

The last submitted patch, 8: test-for-module-help-link-2516690-8.patch, failed testing.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Assigned: Unassigned » joshi.rohit100

Working on this

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs work » Needs review
FileSize
test-for-module-help-link-2516690-15.patch1.87 KB
interdiff-15.txt1.32 KB
naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Reviewed & tested by the community

The above looks good to me. Test passed locally
Thanks!

  • xjm committed bb4206a on 8.0.x 
    Issue #2516690 by mlevasseur, joshi.rohit100: Missing test for "Help"...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

This issue only changes test code, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Nice work!

@mlevasseur, congratulations on your first Drupal core commit credit!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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