TestingParty08: Main help page

partnered with bloo4u

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

changing status

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

Hm. This causes a bunch of failures for me:

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

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

OK, let me dig into it more tonight.

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

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 :)

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) . '">')

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

The last submitted patch failed testing.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

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

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

Looks good now.

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.

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.

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.

The last submitted patch failed testing.

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

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)

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.

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

The last submitted patch failed testing.

The last submitted patch failed testing.

Test slave glitch.

The last submitted patch failed testing.

rerolled against HEAD

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?

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.

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.

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

thanks stella, all those changes incorporated.

Looks good.

The last submitted patch failed testing.

Patch re-roll because #475596: [meta-issue] Fix the unholy abomination that is the welcome screen was committed.

The last submitted patch failed testing.

The last submitted patch failed testing.

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

setting back to RTBC

The last submitted patch failed testing.

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.

The last submitted patch failed testing.

resetting for testbot

The last submitted patch failed testing.

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

Is this a stray comment?

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

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

Still looks good! Somebody commit this puppy, quick

The last submitted patch failed testing.

bad trial run of new testing bot

The last submitted patch failed testing.

Try this AutoBot one more time...

The last submitted patch failed testing.

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

patch attached, doh.

The last submitted patch failed testing.

hrm, rerolled against HEAD.

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

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! :)

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