We really need to get rolling on 100% test coverage for statistics.module. I'll try and come up with a list of things we'll need to test.

Files: 
CommentFileSizeAuthor
#7 441002_2.patch10.1 KBnaxoc
Passed on all environments.
[ View ]
#6 441002.patch9.73 KBnaxoc
Passed on all environments.
[ View ]
#4 441002.patch9.73 KBnaxoc
Passed on all environments.
[ View ]

Comments

cwgordon7’s picture

Issue tags:+Needs tests

From the test coverage data for statistics.admin.inc and statistics.module, the following things most likely need testing:

- "Recent hits" page (entire page) [important]
- "Top pages" page (entire page) [important]
- "Top visitors" page with no available statistics [minor]
- "Top referrers" page (entire page) [important]
- "Access log" page (entire page) [important]
- Using the "access log settings" page to change statistics' settings [moderately important]
- Viewing a node with a certain number of views - does the "1 read" / "@count reads" text show up properly? [moderately important]
- Test proper handling of deleting a user - is the accesslog properly updated to reflect that the user id no longer exists? [minor]
- Proper clearing of day counts and expired access logs [moderately important]
- Statistics' "popular content" block [important]
- Test correct handling of deleting a node - is the node's counter also deleted? [minor]

Hope this helps! :) It would be great to see a few more thorough tests for statistics.

naxoc’s picture

Taking a look at statistics.admin #drupalcon

catch’s picture

Priority:Critical» Normal
naxoc’s picture

Status:Active» Needs review
StatusFileSize
new9.73 KB
Passed on all environments.
[ View ]

"Recent hits" page (entire page) [important]
"Top pages" page (entire page) [important]
"Top referrers" page (entire page) [important]
"Access log" page (entire page) [important]

The above were already written for #659578: Top Pages and Top Referrers reports giving errors

I have (finally) written tests for the rest of the stuff.

Dries’s picture

This looks like a great patch. Some minor issues:

+++ modules/statistics/statistics.test 3 Jan 2010 22:01:37 -0000
@@ -88,12 +86,41 @@ class StatisticsReportsTestCase extends
+    // Get some page and check if the block is displayed

Make sure your comments end with a point (.). There are a couple of instances where you forgot the trailing point.

+++ modules/statistics/statistics.test 3 Jan 2010 22:01:37 -0000
@@ -145,3 +172,140 @@ class StatisticsBlockVisitorsTestCase ex
+   * Tests that cron clears daycounts and expired accesslogs.

Let's write 'day counts' and 'access logs' here.

+++ modules/statistics/statistics.test 3 Jan 2010 22:01:37 -0000
@@ -145,3 +172,140 @@ class StatisticsBlockVisitorsTestCase ex
+    // Wait two secs before running cron, so the access log timer will time
+    // out.
+    sleep(2);

Could use some extra documentation. It's not immediately clear what the timer is for, and why we need to wait 2 seconds.

naxoc’s picture

StatusFileSize
new9.73 KB
Passed on all environments.
[ View ]

Put some missing points in the comments and corrected the wording in comments a bit.

naxoc’s picture

StatusFileSize
new10.1 KB
Passed on all environments.
[ View ]

And.... Uploaded the wrong patch.. Here is the right one

Dries’s picture

Status:Needs review» Reviewed & tested by the community

I'm marking this RTBC because this looks good.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Agreed! :)

Committed to HEAD.

Status:Fixed» Closed (fixed)
Issue tags:-Needs tests

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