Statistics tests for logging are messy if you want to add one. They have indexes on them and all, so when you try to add a simple logging assertion you have to understand a lot of unnecessary logic.

I think a nice thing would be to add a function like assertStatLog().

CommentFileSizeAuthor
#7 statistics-proper_test_coverage-1326698-7.patch3.75 KBiamEAP
#2 drupal-statistics_refactor_log_tests-1326698-2.patch3.75 KBiamEAP
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented

Since removing accesslog (and a number of intermediary, unrelated improvements to the Statistics test suite), I think the qualms raised in the original issue are largely taken care of.

That being said, I think the test coverage is lacking in some places and redundant in others in regards to logging. Specifically:

  • Now that node counts are done via AJAX, there's really no distinction between Cache HIT vs. Cache MISS vs. authenticated user. We shouldn't need to test this three times. If the callback works once, it should work all the time, regardless of context.
  • Instead, it may be more appropriate to check the existence of the statistics.js file and Drupal settings content that actually performs the data POST for the above three contexts. Currently, there's no test guaranteeing that.
iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented
Status: Active » Needs review
FileSize
drupal-statistics_refactor_log_tests-1326698-2.patch3.75 KB

And upon further reflection, checking for the existence of the scripts/settings across cache/uncached/authenticated is a little absurd too.

We just need to ensure that:

  • The callback works as expected with the POST parameters,
  • The JS appears on node pages,
  • The JS does not appear anywhere else.

Patch attached.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented

#2: drupal-statistics_refactor_log_tests-1326698-2.patch queued for re-testing.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented
Title: Re-factor Statistics logging tests » Proper test coverage for Statistics logging
Category: task » bug

Also re-categorizing as a bug and renaming, since tests as they exist now are not fully covering the behavior of the module.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented

#2: drupal-statistics_refactor_log_tests-1326698-2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-statistics_refactor_log_tests-1326698-2.patch, failed testing.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented
Status: Needs work » Needs review
FileSize
statistics-proper_test_coverage-1326698-7.patch3.75 KB

Re-roll of #2.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented
Issue summary: View changes

Minor fixes for readability in English.

iamEAP’s picture

iamEAP CreditAttribution: iamEAP commented
Issue summary: View changes
Status: Needs review » Closed (duplicate)

Aaaand Statistics broke because these tests weren't in. All of these test improvements ended up going in via #2172859: Statistics JS and settings are not included on node pages, which fixed the module too.

Closing as a dupe, now.