Following on from http://drupal.org/node/619584 (roles couldn't be deleted), we need tests for adding, editing and deleting roles.
I'm fairly new to writing tests, so I'm sure there's plenty of improvements to be made.
Wasn't really sure on the best approach to test whether a role has really been deleted: I've tested for a link to edit that role still appearing on the page.

CommentFileSizeAuthor
#8 role-tests-624854-8.patch2.24 KBdrupal_was_my_past
#6 role-tests-624854-6.patch2.16 KBdrupal_was_my_past
#4 role-tests-624854-4.patch2.04 KBdrupal_was_my_past
editing-roles-test.patch2.35 KBmcjim

Comments

lilou’s picture

lilou commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

mcjim’s picture

mcjim commented

Just to note, this patch will fail testing until http://drupal.org/node/619584 is fixed, as the deleting roles bug is one of the things it's testing for.

drupal_was_my_past’s picture

drupal_was_my_past commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
StatusFileSize
new role-tests-624854-4.patch2.04 KB

Re-roll and update patch from #1.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Novice

Thanks for your work on this patch. Here's a few things to fix:

+++ b/modules/user/user.testundefined
@@ -2169,3 +2169,49 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
+ * Test case to test adding, editing, and deleting roles.
...
+   * Test adding, editing, and deleting roles.

The docblock summaries should begin with a third-person verb, e.g., "Tests."

+++ b/modules/user/user.testundefined
@@ -2169,3 +2169,49 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
+class RoleEditTestCase extends DrupalWebTestCase {
...
+  function testAddRole() {

If we're testing adding, editing, and deleting roles, shouldn't the test case name and test method name reflect that? Maybe check other CRUD tests for a good pattern.

+++ b/modules/user/user.testundefined
@@ -2169,3 +2169,49 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
+    $this->assertText(t('The role has been added.'), t('Successful save message displayed.'));
...
+    $this->assertText(t('The role has been renamed.'), t('Successful editing message displayed.'));
+    $this->assertText($edit['name'], t('Edited role name successfully displayed'));
...
+    $this->assertText(t('The role has been deleted.'), t('Successful delete message displayed.'));

Let's make the assertion messages (2nd argument) a little more clear out of context, e.g., "Role added successfully." Also, assertion messages probably don't need to be translated. See #500866: [META] remove t() from assert message for more information.

+++ b/modules/user/user.testundefined
@@ -2169,3 +2169,49 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
\ No newline at end of file

The patch removes the terminal newline from the file. (Put the cursor on the last line and hit enter.)

Also note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch and making the cleanups described above.

drupal_was_my_past’s picture

drupal_was_my_past commented
Status: Needs work » Needs review
StatusFileSize
new role-tests-624854-6.patch2.16 KB

Thanks @xjm! Here's the re-rolled patch with your suggested revisions. I couldn't find another CRUD test naming pattern, so I took a shot at it.

Status: Needs review » Needs work

The last submitted patch, role-tests-624854-6.patch, failed testing.

drupal_was_my_past’s picture

drupal_was_my_past commented
Status: Needs work » Needs review
StatusFileSize
new role-tests-624854-8.patch2.24 KB

Re-roll patch from #6.

drupal_was_my_past’s picture

drupal_was_my_past commented
Assigned: Unassigned » drupal_was_my_past
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

came across, and thought it was odd because i've been working on #6463: Add static caching for user_roles().

rocket_nova, thanks for this work! however, we seem to have these tests already in UserRoleAdminTestCase::testRoleAdministration().

perhaps have a look at them and see if they can be improved? otherwise we can probably close this.

drupal_was_my_past’s picture

drupal_was_my_past commented
Status: Needs work » Closed (duplicate)

@beejeebus Good catch! I'm closing this issue as a duplicate.