Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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().
Comment | File | Size | Author |
---|---|---|---|
#67 | 295982_help_test_12.patch | 3.93 KB | kscheirer |
#65 | 295982_help_test_11.patch | 3.92 KB | kscheirer |
#57 | 295982_help_test_10.patch | 5.79 KB | kscheirer |
#55 | 295982_help_test_9.patch | 5.55 KB | kscheirer |
#51 | 295982_help_test_8.patch | 5.56 KB | kscheirer |
Comments
Comment #1
jhedstrompartnered with bloo4u
This patch adds tests for css, help topics text, and links for modules implementing hook_help().
Comment #2
jhedstromchanging status
Comment #3
catchAdded full stops to a couple of code comments, ran tests, RTBC. No credit on commit please.
Comment #4
catchComment #5
webchickHm. This causes a bunch of failures for me:
Comment #6
catchHmm, weird. I applied this again and it didn't fail for me.
Comment #7
jhedstromI also re-applied the patch and the tests are passing.
Comment #8
webchickOK, let me dig into it more tonight.
Comment #9
kscheirerpatch applied cleanly and all tests passed.
174 passes, 0 fails, 0 exceptions
Comment #10
kscheirerComment #11
dmitrig01 CreditAttribution: dmitrig01 commentedComment #12
kscheirergood 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 :)
Comment #13
kscheirerrolled 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) . '">')
Comment #14
jhedstromThis patch applies, the tests look good, and they all pass.
Comment #16
lilou CreditAttribution: lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #17
catchTest looks good, there's an extra empty line after the long assertion though which needs removing - should be a very quick re-roll.
Comment #18
jhedstromRe-rolled to remove the extra line mentioned in #17.
Comment #19
catchLooks good now.
Comment #20
Dries CreditAttribution: Dries commentedMmm, 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.
Comment #21
catchYeah 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.
Comment #22
kscheirerI 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.
Comment #24
kscheirerShortened the long text assert to 1 line, and rerolled against HEAD to remove fuzz
Comment #25
kscheirertestbot 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)
Comment #26
Senpai CreditAttribution: Senpai commentedI 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.
Comment #27
Senpai CreditAttribution: Senpai commentedComment #28
Senpai CreditAttribution: Senpai commentedComment #29
Senpai CreditAttribution: Senpai commentedThat last one was a stillborn reroll. This is the right one; Marking RTBC.
Comment #31
sheise CreditAttribution: sheise commentedComment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedTest slave glitch.
Comment #36
kscheirerrerolled against HEAD
Comment #37
cwgordon7 CreditAttribution: cwgordon7 commentedI 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?
Comment #38
kscheirerI 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.
Comment #39
stella CreditAttribution: stella commentedThe following coding style items need to be fixed in help_test.module:
* Implementation of hook_help
- you need to add parentheses and a period.There's also some trailing spaces and indentation issues in help.test.
Comment #40
stella CreditAttribution: stella commentedActually #472642: Remove "implementation of" nominalizations from Docblocks just got committed, so instead of
Implementation of hook_help().
you needImplement hook_help().
Comment #41
kscheirerthanks stella, all those changes incorporated.
Comment #42
catchLooks good.
Comment #44
stella CreditAttribution: stella commentedPatch re-roll because #475596: [meta-issue] Fix the unholy abomination that is the welcome screen was committed.
Comment #46
stella CreditAttribution: stella commentedComment #48
kscheirerrerolled against HEAD and incorporated the change stella noted from #475596: [meta-issue] Fix the unholy abomination that is the welcome screen
Comment #49
stella CreditAttribution: stella commentedsetting back to RTBC
Comment #51
kscheirerkeeping 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.
Comment #53
kscheirerresetting for testbot
Comment #55
kscheirerrerolled to keep up with HEAD, someone review and rtbc please? :)
Comment #56
Senpai CreditAttribution: Senpai commentedIs this a stray comment?
// Loading these (and other?) modules will result in failures?
Comment #57
kscheireryeah, 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
Comment #58
Senpai CreditAttribution: Senpai commentedStill looks good! Somebody commit this puppy, quick
Comment #60
deekayen CreditAttribution: deekayen commentedbad trial run of new testing bot
Comment #62
Senpai CreditAttribution: Senpai commentedTry this AutoBot one more time...
Comment #64
kscheirerRerolled 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:
Comment #65
kscheirerpatch attached, doh.
Comment #67
kscheirerhrm, rerolled against HEAD.
Comment #68
catchThis is nice cleanup, lots of commented out lines removed, saner test.
Comment #69
webchickCool. 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! :)