Tracker Module Tests

skywalker2208 - March 27, 2008 - 17:54
Project:Drupal
Version:7.x-dev
Component:tests
Category:task
Priority:critical
Assigned:flobruit
Status:closed
Description

Setup some simpletest for the tracker module and noticed dmitrig01 is doing some work on it as well. Maybe the tests that I created can help dmitrig01 out.

#1

skywalker2208 - March 27, 2008 - 17:58

Forgot to add the test file.

AttachmentSize
tracker.test7.51 KB

#2

webchick - March 28, 2008 - 02:34
Status:active» patch (code needs review)

Marking needs review. No time to do it myself atm.

#3

boombatower - March 30, 2008 - 04:39

I was hoping dmitrig01 would post his and we could compare/merge, but that doesn't seem to be happening so I'll review yours tomorrow.

#4

dmitrig01 - April 1, 2008 - 16:22
Version:6.x-1.x-dev» 7.x-1.x-dev
Priority:normal» critical
Assigned to:Anonymous» dmitrig01

Here they are, my very special tracker tests.

AttachmentSize
tracker_tests.patch1.02 KB

#5

dmitrig01 - April 1, 2008 - 16:22

With the tests this time!

AttachmentSize
tracker_tests.patch10.04 KB

#6

boombatower - April 2, 2008 - 00:11
Priority:critical» normal
Status:patch (code needs review)» patch (code needs work)

@dmitrig01: I noticed a few things that I would appreciate if they were cleaned up.

  • I not sure the drupalCreateNodes method is necessary, and if it is I would like to have others approve it. The comment has a bit of random spacing and is missing periods.
  • It has been agreed that tests should have documentation on the methods. If you would add that it would be appreciated.
  • All comments should end with a period.
  • All displayed messages should be wrapped with t().
  • Most tests use the $message parameter with asserts to make it more clear what the test is doing. That isn't necessary, but nice.
  • It isn't necessary to implement tearDown just to call parent.
  • It seems the following code is common to all the test methods and should be moved to setUp.
    <?php
    // Enable the comment and tracker modules
    $this->drupalModuleEnable('comment');
    $this->drupalModuleEnable('tracker');
    // Create some nodes, a user, etc.
    $data = $this->trackerPostNodes();
    ?>

You can look at the tests that I have reviewed to see an example of what we are looking for.

The changes are minor, but the test as a whole looks nice.

Thanks.

#7

catch - May 1, 2008 - 13:48
Project:SimpleTest» Drupal
Version:7.x-1.x-dev» 7.x-dev
Component:Code» tracker.module
Priority:normal» critical
Status:patch (code needs work)» patch (code needs review)

http://drupal.org/node/253500 was duplicate.

#8

Dries - May 10, 2008 - 07:47
Status:patch (code needs review)» patch (code needs work)

I think boombatower's review still stands.

#9

flobruit - May 13, 2008 - 22:45
Assigned to:dmitrig01» flobruit
Status:patch (code needs work)» patch (code needs review)

I agree with boombatower's comments. The patch from #5 is also not structured like any existing core texts:

* Its use of xpath is very powerful, but also very obscure.
* Direct calls to curlExec() are a big no-no, they belong in the testing framework, not in the tests themselves.
* Why do we need to test everything on multiple nodes at once? Unless we're looking for a specific behavior when dealing with multiple nodes, we shouldn't test on multiple nodes. There's no need to add unrelated parameters to a test.

Here's a simpler test that covers the same functionality.

AttachmentSize
tracker.test.patch4.13 KB

#10

flobruit - May 13, 2008 - 22:50

Although the patch from #1 needs some clean-up, it covers some code that is not covered by dmitrig01's patch or mine. I'll work on merging these tests.

#11

flobruit - May 14, 2008 - 00:45

After a closer look, the only case checked by the patch from #1 that I was missing was that unpublished nodes should not show up in the tracker listing.

AttachmentSize
tracker.test.patch4.75 KB

#12

boombatower - May 14, 2008 - 22:25
Status:patch (code needs review)» patch (reviewed & tested by the community)

This patch is much cleaner than the original. It passes on my dev box and seems to be much more consistent with other tests.

I removed several page content outputs that seem to be left over from development and cleaned up some white-spacing.

Good work flobruit.

AttachmentSize
tracker.test.patch4.8 KB

#13

boombatower - May 14, 2008 - 22:45
Component:tracker.module» tests

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

#14

Dries - May 15, 2008 - 20:52
Status:patch (reviewed & tested by the community)» fixed

Committed to CVS HEAD. Thanks all.

#15

Anonymous (not verified) - May 29, 2008 - 20:53
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.