Posted by catch on August 17, 2008 at 8:40pm
9 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | task |
| Priority: | normal |
| Assigned: | jhedstrom |
| Status: | needs work |
Issue Summary
Write a small module that implements hook_comment() so you can test it.
Comments
#1
#2
#3
#4
Sorry, I've wanted to do this, but all the time stuff getting between it.
#5
If you still want to do it, feel free to assign yourself to that task, if you're able to do it in the next month or so ;)
#6
I have some time for this if nobody else is working on it.
#7
Yeah, would be great, I don't have any dev time atm :(
#8
Ok, here's a patch that adds a comment_test module. I wasn't sure where test modules for modules should go. I know for .inc files, the tests all go in modules/simpletest/tests...this patch adds comment_test.module and comment_test.info into the modules/comments directory, where comments.test is currently located.
#9
I'm pretty sure all the test modules go into modules/simpletest/tests. That shouldn't stop this getting reviewed though.
#10
Re-rolled to move comment_test.* files to the simpletest/tests directory.
#11
The last submitted patch failed testing.
#12
Re-rolle to fix the exceptions.
#13
The last submitted patch failed testing.
#14
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#15
Comment tests pass with the patch applied, and it looks fine to me. Not sure how well it will survive if we de-$op hook_comment though, but that should probably be dealt with in the patch which does this.
#16
We already have a
function assertFileHooksCalled($expected)in file.test and I'm not real jazzed about comment module also making its own assertion like this. We're going to end up with 90 of these, one in each major hook subsystem. Yuck.Is it possible to abstract this out to an
assertHookCalled($expected)which both file and comment modules tests use?Otherwise, code style looks good.
#17
I like the idea of abstracting this function out, but I'm at a bit of a loss on how that would be possible, given the way hooks work. Currently, in the assertCommentHookCalled method, the first line, and the last line, contain code specific to the comment_test.module:
<?php$actual_count = count(comment_test_get_calls($op))
?>
and
<?php// Reset operation
comment_test_reset_op($op);
?>
in order to abstract out a assertHookCalled, we'd need a uniform method for test modules to set and unset flags when hooks are called...perhaps something like:
<?phpfunction {module_name}_get_hook_calls($op) {}
function {module_name}_reset_hook_calls($op) {}
?>
the abstracted method would then look like:
<?phpfunction assertHookCalled($test_module_name, $op) {...}
?>
Is this the direction I should go with this patch?
#18
additional hook_comment is not there anyonemore its hook_comment_$ops
so the module and the assert Message are not working at the moment
#19
Moving this out of the critical bugs queue - see #607038: Meta issue: fix gaps in code coverage.