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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

skywalker2208’s picture

FileSize
7.51 KB

Forgot to add the test file.

webchick’s picture

Status: Active » Needs review

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

boombatower’s picture

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.

dmitrig01’s picture

Version: 6.x-1.x-dev »
Assigned: Unassigned » dmitrig01
Priority: Normal » Critical
FileSize
1.02 KB

Here they are, my very special tracker tests.

dmitrig01’s picture

FileSize
10.04 KB

With the tests this time!

boombatower’s picture

Priority: Critical » Normal
Status: Needs review » 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.
    // 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.

catch’s picture

Project: SimpleTest » Drupal core
Version: » 7.x-dev
Component: Code » tracker.module
Priority: Normal » Critical
Status: Needs work » Needs review
Dries’s picture

Status: Needs review » Needs work

I think boombatower's review still stands.

floretan’s picture

Assigned: dmitrig01 » floretan
Status: Needs work » Needs review
FileSize
4.13 KB

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.

floretan’s picture

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.

floretan’s picture

FileSize
4.75 KB

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.

boombatower’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.8 KB

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.

boombatower’s picture

Component: tracker.module » tests

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks all.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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