I had two problems: a bunch of failures and it was one big block of code. Now it's five tests, sliced up and they all pass.

CommentFileSizeAuthor
cleanup_comment_test.patch18.89 KBchx

Comments

dries’s picture

dries commented

Why did you create multiple classes for the comment tests? I'd think functions within a single class are more proper.

chx’s picture

chx commented

Nope, the test runner runs per class so you can run in smaller chunks, if something fails, it's easier to isolate etc

catch’s picture

catch
he/him
Primary language English
commented

Doesn't conflict with this one but fwiw http://drupal.org/node/246392 tidies up a couple of issues with setCommentForm - which doesn't fail, or mask any failures, but isn't right either.

@Dries, there's an attempt to get some conventions for test classes (and everything else really) here: http://groups.drupal.org/node/11020

fwiw I think more, smaller classes (as long as this doesn't go too far) makes it a lot more readable and easier to grok what the tests are doing. As tests become more comprehensive some (especially in something like system.module) will be huge.

keith.smith’s picture

keith.smith commented
Status: Needs review » Needs work

Setting to CNW.

This is creating nodes of type 'story', which was redefined to 'article' in D7 (though apparently that doesn't cause the tests to fail). Plus, at chx's request, I'm going to work on the comments a bit when I have a second.

boombatower’s picture

boombatower commented
Component: comment.module » tests

Change component is relation to http://drupal.org/node/253744.

boombatower’s picture

boombatower commented

The patch doesn't apply to HEAD any longer and needs to be re-rolled.

I would vote for not breaking it into separate classes. It already has separate functions for each of the areas and separate classes are a bit of an over kill and add redundant getInfo information along with setUp as well.

As a note this was broken with a recent patch to core. It would be handy if the persons involved would help fix this since they know what was changed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Clean up comment test » Comment tests fail (+ cleanup)
Category: task » bug
Priority: Normal » Critical

Still an issue. Tests that don't fail are critical.

boombatower’s picture

boombatower commented

Anyone have any knowledge of what broke this. Short of brute forcing it. I know it passed when committed to HEAD, not sure why the tests aren't being kept up to date with core.

Makes for a lot of work maintaining tests, as it was when trying to get them all the pass the first time.

catch’s picture

catch
he/him
Primary language English
commented

It was fixed here: http://drupal.org/node/266465 - the article > story change hadn't been updated in the test, but that was apparently not what caused the failure. Either way it's RTBC now.

boombatower’s picture

boombatower commented
Status: Needs work » Fixed

Looks like the test passes.

catch’s picture

catch
he/him
Primary language English
commented
Title: Comment tests fail (+ cleanup) » Comment test cleanup
Category: bug » task
Priority: Critical » Minor
Status: Fixed » Needs work

There's still a patch for general cleanup here. Downgrading status though.

boombatower’s picture

boombatower commented

Doesn't apply to head and still debatable whether we want to split them up into classes.

Already several methods and works great.

I'm for closing this issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Closed (works as designed)

Fair enough.