We need to test the main administration help page (admin/help) to make sure it works properly.

Suggested assertions:

- CSS is added.
-

<?php
'<h2>' . t('Help topics') . '</h2><p>' . t('Help is available on the following items:') . '</p>'
?>

text appears.
- Working links appear for all enabled modules that implement hook_help().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

FileSize
2.37 KB

partnered with bloo4u

This patch adds tests for css, help topics text, and links for modules implementing hook_help().

jhedstrom’s picture

Status: Active » Needs review

changing status

catch’s picture

FileSize
2.37 KB

Added full stops to a couple of code comments, ran tests, RTBC. No credit on commit please.

catch’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. This causes a bunch of failures for me:

Link properly added to Block (admin/help/block)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Blog (admin/help/blog)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Color (admin/help/color)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Comment (admin/help/comment)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Database logging (admin/help/dblog)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Filter (admin/help/filter)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Help (admin/help/help)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Menu (admin/help/menu)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Node (admin/help/node)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Poll (admin/help/poll)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to System (admin/help/system)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to Taxonomy (admin/help/taxonomy)	Other	help.test	55	HelpTestCase->testHelp()	
Link properly added to User (admin/help/user)	Other	help.test	55	HelpTestCase->testHelp()
catch’s picture

Hmm, weird. I applied this again and it didn't fail for me.

jhedstrom’s picture

I also re-applied the patch and the tests are passing.

webchick’s picture

Status: Needs work » Needs review

OK, let me dig into it more tonight.

kscheirer’s picture

Status: Needs review » Reviewed & tested by the community

patch applied cleanly and all tests passed.
174 passes, 0 fails, 0 exceptions

kscheirer’s picture

Status: Reviewed & tested by the community » Needs review
dmitrig01’s picture

Status: Needs review » Reviewed & tested by the community
kscheirer’s picture

Status: Reviewed & tested by the community » Needs work

good mystery! This patch worked great on my test server, and failed with the same errors webchick saw on my neighbor's server. Good thing we were at the BADCamp testing party and able to check things out pretty quick.

this patch is testing for <a href="/admin/help/$module"> links, but depending on your server settings, the links might not come out like this.
checking against something like <a href="'.url('admin/help/' . $module) . '"> allows the tests to pass on our setups.

flobruit and chx also mentioned that having a help_test module that just enables hook_help might be more appropriate to test that help links appear, and actually clicking the link and getting a positive response would be more specific.

I will try to post a new patch soon, unless someone beats me to it :)

kscheirer’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

rolled a new patch.

changes:
added a help_test module that just enables hook_help

added a test for the intro text on admin/help, according to http://acquia.com/files/test-results/modules_help_help.module.html this was the only untested code in the help module.

changed the tests that were failing to assertLink()s. Not sure if thats considered preferential to assertRaw('<a href="'.url('admin/help/' . $module) . '">')

jhedstrom’s picture

This patch applies, the tests look good, and they all pass.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
catch’s picture

Category: bug » task
Status: Needs review » Needs work

Test looks good, there's an extra empty line after the long assertion though which needs removing - should be a very quick re-roll.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
4.43 KB

Re-rolled to remove the extra line mentioned in #17.

catch’s picture

Component: tests » help.module
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs tests

Looks good now.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Mmm, the text in the asserts can be made shorter. We don't need to assert on the entire 3 line paragraph.

I'm not sure we need the test module. We can use any of the core modules do perform the same test.

There is some code commented out in an odd way.

catch’s picture

Yeah that all makes sense. Except the oddly commented code is actually being removed by the patch rather than put in, I had to double check that a few times.

kscheirer’s picture

Status: Needs work » Needs review

I think we do need the test module. We are testing the functionality of hook_help(), we aren't testing whether poll or some other core module implements it correctly. The assert could check against shorter text though.

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Shortened the long text assert to 1 line, and rerolled against HEAD to remove fuzz

kscheirer’s picture

testbot not updating, but patch passed all tests

http://testing.drupal.org/pifr/file/1/help.test_2.patch

(here's the issue for testbot not updating : http://drupal.org/node/372947)

Senpai’s picture

FileSize
4.4 KB

I caught a typo in the test's description, and also tried to change the help.css test to xpath for actual verification that the HTML was, in, fact, there. That didn't work so good, so I left it alone. All looks good.

Senpai’s picture

Status: Needs review » Needs work
Senpai’s picture

Status: Needs work » Needs review
Senpai’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.42 KB

That last one was a stillborn reroll. This is the right one; Marking RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sheise’s picture

Category: task » bug
Status: Needs work » Reviewed & tested by the community
FileSize
4.42 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community
Damien Tournoud’s picture

Test slave glitch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
4.3 KB

rerolled against HEAD

cwgordon7’s picture

I don't quite understand the significance of the test module - why can't we just test existing core modules? What does the test module add to the test?

kscheirer’s picture

I think my comments from #22 still apply - we want to test the functionality of hook_help(), not whether a particular module like poll or blog implements it. But if the consensus is that we don't need it, I'm fine with removing it.

stella’s picture

Status: Needs review » Needs work

The following coding style items need to be fixed in help_test.module:

  • Line 10: * Implementation of hook_help - you need to add parentheses and a period.
  • Lines 15, 17: in D7, string concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

There's also some trailing spaces and indentation issues in help.test.

stella’s picture

Actually #472642: Remove "implementation of" nominalizations from Docblocks just got committed, so instead of Implementation of hook_help(). you need Implement hook_help().

kscheirer’s picture

Status: Needs work » Needs review
FileSize
4.34 KB

thanks stella, all those changes incorporated.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
5.7 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
3.05 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
4.38 KB

rerolled against HEAD and incorporated the change stella noted from #475596: [meta-issue] Fix the unholy abomination that is the welcome screen

stella’s picture

Status: Needs review » Reviewed & tested by the community

setting back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Assigned: Unassigned » kscheirer
Status: Needs work » Needs review
FileSize
5.56 KB

keeping up with HEAD and #372743: Body and teaser as fields, which accidentally added a duplicate assertion. Trimmed the list of modules we test to only include ones that actually implement hook_help() - some of the new Fields modules (list, number) don't bother. I think it's fine, since their parent field.module has help.

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review

resetting for testbot

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
5.55 KB

rerolled to keep up with HEAD, someone review and rtbc please? :)

Senpai’s picture

Status: Needs review » Needs work

Is this a stray comment?

// Loading these (and other?) modules will result in failures?

kscheirer’s picture

Status: Needs work » Needs review
FileSize
5.79 KB

yeah, that and // 'administer blocks', 'administer site configuration', a few lines below.

attached patch also removes those, and adds the required newline at the end of help.test

Senpai’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good! Somebody commit this puppy, quick

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

deekayen’s picture

Status: Needs work » Reviewed & tested by the community

bad trial run of new testing bot

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Senpai’s picture

Status: Needs work » Needs review

Try this AutoBot one more time...

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review

Rerolled against HEAD with a few changes.

I got rid of the help_test.module, since no one seems to like it. I still think it's a little silly to require poll and blog module's help, but it works. No other barriers to getting committed have been posted to this issue.

D7 now uses the seven theme so:

  • updated the page title html we're looking for
  • stop checking for breadcrumbs, since seven doesn't use them
kscheirer’s picture

FileSize
3.92 KB

patch attached, doh.

Status: Needs review » Needs work

The last submitted patch failed testing.

kscheirer’s picture

Status: Needs work » Needs review
FileSize
3.93 KB

hrm, rerolled against HEAD.

catch’s picture

Category: bug » task
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

This is nice cleanup, lots of commented out lines removed, saner test.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool. I always get a little nervous about these assertions that are so tied to our user interface text, but OTOH, this new code is light-years beyond the code that's there now.

Committed to HEAD! Great work! :)

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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