Tests for node_access

first patch

this test does not cover yet:
1. hook_access
2. node_access table

the new patch covers hook_access and hook_node_access_records

but i'm not sure whether has to be such many tests, but i try to cover a lot of things, so this is definitve a unit test and not functional test

this patch applies

rerole

the component tests are not there anymore

Resubmit for retest.

Re-roll of #13 which removes unwanted newline from a function.

Oops, seems I missed one.

Discard previous patch, also corrected indentation of the array in the one attached to this comment.

Needs to be converted to PDO.

here is a new version

the following stuff changed:

1. All Asserts have upper case at the beginning.
2. Only use " very it is needed.
3. use dbtng for delete of permissions

thx to jsosmers.

here is a working version.

Patch applied fine, tests run fine and code looks OK.

Some things:

+    // Create a admin user, who has access to all content.

Create an admin user...

+    // Create a user without access content perission.

permission.

+    // Remove access content for authenticated users!, because all users created  are authenticated.

two spaces between created and are, but we should probably generally make this more clear.

Let's get rid of $settings_1/2/3 and $access_1/2/3... -- those are never re-used, so I don't see a reason they can't all be just $settings and $access, right?

+++ modules/simpletest/tests/node_access_test.module

This new file should have a "@file" PHPDoc definition at the top. See other core files for an example.

As a general comment around variable names and such, you are testing the most complex system that Drupal has outside of the Form API. These tests represent a tremendous boon not only because they help us not to break things, but also because they give people a complete walkthrough of the logic of node access and how it applies.

Therefore, I'd really like to see more descriptive names than $web_user and $web_user_2 and $node_1 and $node_2, etc. For example, $web_user_no_access is a perfect name, and very clearly describes what that user is there for. Can we please rename the other variables to be similarly clear?

so this is a much better version:

butter naming of variables, not access_1/.. etc.

For example node_X to $node_unpublished_author_web_user.

There is also a fix which jsomers catched.

There should be someone reviewing the patch who really really understands how the node access system works.

a magic hand removed parent:setUp from nodeSave tests :)

next version of the patch

This took me way long to fix up. I'd like to blame it on DBTNG's inability to show me the actual SQL it is running. Anyway, the changes are.

1. Minor docs update. Change boolean to int or DBTNG cries. I already updated the example module accordingly.
2. Some minor cleanups for the tests that exercise the top part of node_access()
3. Completely revamped the tests at the end which exercise the hook_node_grants and hook_node_access_records system.
4. One line addition to setup() method in drupal_web_test_case.php. Rebuild node access. Tests obviously can't wait for an admin to press the button.

This doesn't test the node_access_view_all_nodes function which is called by node_db_rewrite_sql. This is why all your tests are passing here even though there's a typo in that function that breaks the query when grants get involved. (That typo has been fixed in #309007: Add drupal_alter() after hook_node_access_records(), which you just RTBC'd, and is effectively tested there in conjunction with the 'Node RSS Content' test.) I'm only mentioning this here in case you think node_access_view_all_nodes should be explicitly tested in this suite but you missed it.

1. + // Remove access content for authenticated users!, because all users created are authenticated. You shouldn't use '!' in the middle of a sentence. Also remove the new line after this comment.

2. The fact that we have to db_delete('role_permission') is a hack. It is outside the scope of this patch, but I think we should, at least, a TODO to the code and create an issue to define a better API for manipulating roles.

3. Typo: contenttyp.

4. I think it woudl be good to reference node_access_test.module from the documentation in node.test. It might be useful to include a one or two-line summary in node.test about what the code in node_access_test does.

Few other comments (all minor):

• modules/node/node.test: @see should always be at the beginning of a line, never inline in other comments, and never with trailing punctuation. It should also be in the function comment block, rather than in the middle of the function code.
• modules/node/tests/node_access_test.module: there's a number of trailing spaces, and there's incorrect indentation in node_access_test_node_access_records()

With these and Dries comments, setting back to needs work. :(

This will definitely need a re-roll now that #309007: Add drupal_alter() after hook_node_access_records() is committed.

Add tag

Marked #710606: Node access tests are incomplete as a duplicate.

Subscribing