WANTED: SimpleTests for Forum Access

Fantastic, that'll be a great help!

We have tests for D7 — I don't know if they could serve as a starting point?

Please assign yourself. I can only unassign you, not assign you.

The maintainer can assign to co-maintainers, but not to anyone.

Wow, this is fantastic!

I have only some minor issues at this point:

-- Avoid anonymous functions because these aren't available in PHP 5.2.

-- Please set your editor to indent two spaces rather than tabs. Tabs are frowned upon in the Drupal community. Run Coder 6.x-2.x-dev, which will help you fix this and other minor issues.

Thank you for cleaning up the coding style!

I had to raise the time limit for the tests to complete (#523878: Fatal error: Maximum execution time of 180 seconds exceeded):

  public function setUp() {
    $this->timeLimit = 500;
    parent::setUp('forum_access', 'forum', 'comment', 'acl');
  }

Do all the tests pass for you? I get

#1: 3759 passes, 52 fails, and 0 exceptions
#2: 3495 passes, 39 fails, and 0 exceptions
#3: 3330 passes, 45 fails, and 8 exceptions
#4: 3052 passes, 29 fails, and 12 exceptions
#5: 3281 passes, 47 fails, and 8 exceptions
#6: 3024 passes, 25 fails, and 8 exceptions
#7: 949 passes, 36 fails, and 22 exceptions

The fails can be failures in the tests or in FA, or they could somehow be related to my environment...

(Looking for the few failures among the many passes is quite a pain — if we only had #912460: Tired of searching for those few red and yellow rows in a sea of green......)

So you get the same results at this point?

I've fixed the 'href' notice and pushed the fix.

The fail on line 504 is bogus. If you look at the Forums drop-down, then you see that the forum is not listed. Anyone with the 'create forum topics' permission can call up /node/add/forum and /node/add/forum/TID with any TID (even non-existing ones). The trick here is that FA removes the forums from the Forums drop-down where the user doesn't have the Post permission. I'm not sure whether you can test the content of the drop-down...

The fail on line 527 is the logical consequence of the former one. You are testing for "not authorized", but this is not what happens. If you look at the screenshot, you'll see that Drupal comes back with a 'Forums field is required.' error, because the forum wasn't preselected (because it's not on the list).

With these fixes, #7 should pass without a hitch.

I've looked at your testForumAccessAllowed() function, but it's difficult to tell in every case what it does. I think it's easier to look at the fails and their screenshots (the "Verbose Messages") and then consider what is ging wrong.

For example #6 line 610 and 624: It's not the edit link that's missing, but the node is not viewable. You have successfully tested in 464 that "Anonymous should NOT be allowed to view this topic: http://d6.local/node/21". After that, 610 and 624 don't make sense, because

* the 'access content' and 'access comments' permissions AND View to be able to see this forum and its content at all,

IOW, without View all the other permissions have no effect.

With #1055672: hook_link_alter quirks (removing links and causing other modules to alter twice) applied I get the exact same counts.

I get the same numbers.

Re Test #6 (see screenshot):

What is the difference between "moderator" and "database moderator"?

If "(database) moderator status" is TRUE, then why should the user NOT be allowed to edit the page in line 605/666/685/891? or the delete page in line 960?

What is the difference between "forum grants" and "database forum grants"?

Line 747/815: Why "For this user there should NOT be an reply-link on this page for a new comment: http://d6.local/node/5" if
"Set forum grants are: authenticated_view, authenticated_create" and
"Database forum grants are: authenticated_view, authenticated_create"

Line 581&583/642&644/656&658: If it's ok that "GET http://d6.local/node/20 returned 403", then why "For anonymous there should be an edit-this-topic-link on the page: http://d6.local/node/20"?

Thank you for the explanation. It seems that there should be a few tests at the beginning of each test run to ensure that the variables and the database are in sync, rather than making the human compare visually in case of fails. When looking at a fail, I find it a pain that I have to start by comparing the two (actually two times two) every time.

I've added the option to activate the Devel Node Access module (and happened to find a minor buglet in there, a notice from theme_username(), which I've fixed and committed to Devel).

I've also fixed some trailing spaces and some typos in strings and comments. The Drupal Coding Conventions call for ending comments with a dot and wrapping them before col 80. Would you take care of that? I know that FA doesn't conform in all aspects, but I'd like any incoming code to not increase the body that needs fixing.

Your tests uncovered some bugs related to priorities of contradictory settings (like Post without 'post comment' or Moderator without 'access content') — great job! I don't think any of these are critical. I've fixed them and I'm attaching a patch with the fixes and the updated forum_access.test file. The patch is against the current 6.x-1.x head.

I'm confused though: Now that everything is fixed, I only get 546 passes (and no fails) in each test, for a grand total of 3822 passes. Does the number of assertions somehow depend on how the test is going? Each fail did generate a few additional records, but that cannot explain the huge difference.

@daffie: Would you please fix/explain the remaining issues in #17?

@daffie: Are you still there?

What, you're down to 55 passes? And I thought 546 was low...

I'm up to my neck in #1172646: Subscriptions for D7 right now — I'll look into this as soon as ALPHA5 is out.

Hey daffie,

I'm sorry for taking so long to get back here...

Thanks for fixing lots of the comments!

- to check if the variables are in sync with the database. Point taken! Good idea. Have no clue why I did not think of that myself. I have changed the code for that.

Thanks, this is a great improvement!

In the file forum_access.module you made a fix, that has a side affect.

You mean the following?

diff --git a/forum_access.module b/forum_access.module
index ebbb0dd..7fe7a24 100644
--- a/forum_access.module
+++ b/forum_access.module
@@ -523,6 +523,10 @@ function forum_access_access($tid, $type, $account = NULL, $administer_nodes_see
   }

   if (!isset($cache[$account->uid][$tid][$type])) {
+    if (!user_access('access_content')) {
+      return $cache[$account->uid][$tid][$type] = FALSE;
+    }
+
     $roles = array_keys($account->roles);
     $result = db_result(db_query("SELECT tid FROM {forum_access} WHERE rid IN (". db_placeholders($roles) .") AND grant_". $type ." = 1 AND tid = %d", array_merge($roles, array($tid))));

There's a typo in my code: it must be 'access content', not 'access_content', of course!

After the forum is created, you get back to the content management forums page "admin/content/forum". The page is saying that a new forum has been created. Only the new forum has no link attached to it and the link "edit forum" is also not there. So The script cannot determine what the forum_id is for the new forum. The problem only exists with simplest.

Hmm, this worries me. My typo caused forum_access_access() to return FALSE in most cases, and the tests' only reaction was to reduce the number of passes. You've added a debug message, but creating a forum with a response of 200 and not getting an "edit forum" link is an error. I've now added an assert to check that we were able to detect the $forum_tid. Silently skipping most of the tests hides the error and possibly other fails.

Ideally, the sum of the passes and fails should be constant. The test should always run the same sequence of asserts, with each one either passing or failing. One type of shortcut makes sense though: if the consequence of one failure is that the next asserts will necessarily fail, too, or can't even be executed properly, then it's OK to bypass them. However, if we shrink down to "55 passes, 0 fails, and 0 exceptions," then something is wrong. All that this says is that none of the 55 asserts that we checked failed. But we have thousands more that we haven't checked, so "55 passes, 0 fails, and 0 exceptions" tells us just about nothing.

Fixing my typo brings me back to:
#1: 4603 passes, 72 fails, and 0 exceptions

It turns out that editing comments requires the 'post comments' permission. Adding that to the Forum Moderator role and enabling that role on comment/%/edit brings the count down to
#1: 4603 passes, 1 fails, and 0 exceptions

That last fail is due to anonymous editing a comment and that comment disappearing for moderation. The subsequent check for the 'delete' link fails, because the comment is gone.

Your interpretation of 'post comments without approval' is incorrect. That permission has no effect, if the user doesn't have the 'post comments' permission. Thus, it's not an "OR" thing. I actually had to add both permissions to the moderator role to cover all edge cases.

I don't understand the following change:

<           preg_match('/admin\/content\/forum\/edit\/forum\/(?<forum_id>\d+)/', $html, $matches, 0, $position);
---
>           preg_match_all('/admin\/content\/forum\/edit\/forum\/(?<forum_id>\d+)/', $html, $matches);

I don't think this can work without changing the surrounding code and I'm ignoring it for now. Please explain if this needs to go in.

Above are some of the points worth noting. With some more tweaking, both in the test code and in the production code I get this to

#1: 4851 passes, 0 fails, and 0 exceptions
#2: 4566 passes, 0 fails, and 0 exceptions
#3: 4214 passes, 0 fails, and 0 exceptions
#4: 3943 passes, 0 fails, and 0 exceptions
#5: 4102 passes, 0 fails, and 0 exceptions
#6: 3929 passes, 0 fails, and 0 exceptions
#7: 1498 passes, 0 fails, and 0 exceptions

I think that's pretty good. Here's the patch against the current -dev version. Please review my changes and let us know what you think.

Hi daffie,

I have changed the sequence so that it would be more easy for you to spot the problem. After you found the problem I would restore the original sequence.

I don't understand what "sequence" you're referring to. What should we restore?

I have reviewed the code changes for the forum_access.test file and I can see only one problem: Some variables are not initialised before they are used in an if statement. Is that a problem? ($test_direct_link & $page_has_delete_button in the function testForumAccessDeleteTopic)

They're only used inside empty(), which treats uninitialized variables as empty, i.e. returns TRUE, without any notice. Also, empty() is evaluated directly in PHP, so you don't have the overhead of a function call. IOW, the code is correct, it's just a matter of taste. However, for the sake of uniformity of the code, I'm switching the two variables to initialized.

At this moment we only test the forum functionality. What do we do with quote and advanced forum modules? (maybe others?)

Writing and maintaining tests for interactions with other contrib module maintained by other teams is difficult. Any of the involved modules could break any test at any time, not necessarily because it has introduced a bug, but because it has changed some of its internal workings. This would be chasing a moving target.

I've never actually seen AF. Have you ever tried running the tests with AF installed?

I am happy that the simpletest helped to fix some bugs in this module. Can my name be added as co-auther for this patch when the patch is commited to the forum_access module?

Yes, of course, you're the main author.

On the module page of forum_access you claim that "Forum Access is compatible with the core Forum module, Advanced Forum module".

Yes, that means that my intention is for them to be compatible. If issues are reported, then I will work to resolve them (as I've done in the past), or change the text if it turns out that maintaining compatibility becomes impossible or unreasonable.

Here's your commit: http://drupalcode.org/project/forum_access.git/commit/a712e79

Thank you very much for your work!

I will release 6.x-1.7 shortly.

Hmm, I wonder why the testbot gets different pass counts:
http://qa.drupal.org/pifr/test/193549

Forum access functionality #7 1203 passes, 0 fails, and 0 exceptions
Forum access functionality #6 3025 passes, 0 fails, and 0 exceptions
Forum access functionality #4 3035 passes, 0 fails, and 0 exceptions
Forum access functionality #5 3147 passes, 0 fails, and 0 exceptions
Forum access functionality #3 3227 passes, 0 fails, and 0 exceptions
Forum access functionality #2 3489 passes, 0 fails, and 0 exceptions
Forum access functionality #1 3691 passes, 0 fails, and 0 exceptions

vs. the counts in #25:

#1: 4851 passes, 0 fails, and 0 exceptions
#2: 4566 passes, 0 fails, and 0 exceptions
#3: 4214 passes, 0 fails, and 0 exceptions
#4: 3943 passes, 0 fails, and 0 exceptions
#5: 4102 passes, 0 fails, and 0 exceptions
#6: 3929 passes, 0 fails, and 0 exceptions
#7: 1498 passes, 0 fails, and 0 exceptions

Thank you for looking into it, daffie!

I've answered in the other thread, and rfay's answer to my support request #1347808: Neglecting to use url() in asserts leads to fails on qa.d.o may provide additional insight.

How can the "Provide verbose information when running tests" setting have an influence on the number of asserts? Is the test code somehow checking and behaving differently depending on that setting?

I'm still puzzled by this and don't know whether the missing asserts mean less thorough testing...

Ah, yes

1121 + 377 = 1498

that makes sense.

I've tried the same #7 but this time with clean URLs turned off, and I get:

1125 + 379 = 1504

The slight increase is due to the additional code to turn the clean URLs off.

So, we're pretty consistent — it's only the qa.d.o that has an additional 82 passes up to 1203.

With D7, the numbers are different, too: test #1 has 257 passes locally and 235 on qa.d.o. However, test #0, which essentially just runs the forum.test with FA and ACL installed gets the same 615 passes locally and on qa.d.o. This looks as if the core tests were more predictable...

I think we'll just have to accept the varying numbers. As long as they don't drop dramatically, we should be fine.

See #1356390: Edit link not displayed on own comments, 6.x-1.7 — I've added a test for the 'edit' link on the reply.

On the way I encountered this:

+++ b/forum_access.test
@@ -923,8 +941,9 @@ class ForumAccessTestCase extends DrupalWebTestCase {
-            preg_match_all('/node\/(?<topic_id>\d+)#comment-(?<comment_id>\d+)/', $this->url, $matches);
-            return (array_key_exists('comment_id', $matches) ? intval($matches['comment_id']) : 0);
+            //preg_match_all('/node\/(?<topic_id>\d+)#comment-(?<comment_id>\d+)/', $this->url, $matches);
+            //return (array_key_exists('comment_id', $matches) ? intval($matches['comment_id']) : 0);
+            return TRUE;

I guess the intention was to return the cid of the new reply, but this always returned 1. The return value used to be ignored, so it didn't matter. I'm just returning TRUE now and let the caller guess the cid. Can you get this to actually work?

The other case seems bogus

+++ b/forum_access.test
@@ -933,12 +952,13 @@ class ForumAccessTestCase extends DrupalWebTestCase {
-            preg_match_all('/node\/(?<topic_id>\d+)#comment-(?<comment_id>\d+)/', $this->url, $matches);
-            return (array_key_exists('comment_id', $matches) ? intval($matches['comment_id']) : 0);
+            //preg_match_all('/node\/(?<topic_id>\d+)#comment-(?<comment_id>\d+)/', $this->url, $matches);
+            //return (array_key_exists('comment_id', $matches) ? intval($matches['comment_id']) : 0);

It's probably better not to return anything, because that would only create useless follow-up errors.

FA now has recurring tests, which means that our test suite runs automatically as soon as a patch is posted. This is really really nice and it takes maintenance to an entirely new level.

Thank you for looking into the issue!

I agree that it's ugly, but it avoids duplicating the comment.module code. Duplicate code and other types of redundancy have cost me months of my life, and those were painful hours, every one of them. Duplicating things (code or data) is the worst crime that you can commit in software engineering.

Pretty clearly has to stand back when it comes to avoiding duplication, and the type that you propose (copying code without stating its origin, and modifying it so that it's not a recognizable duplicate anymore, even though the semantics must be preserved and kept in synch) is a particularly nasty one. Beauty is only skin-deep, but duplication is a serious illness that will hurt you again and again, as long as you tolerate it.