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.
Comment | File | Size | Author |
---|---|---|---|
#8 | role-tests-624854-8.patch | 2.24 KB | drupal_was_my_past |
#6 | role-tests-624854-6.patch | 2.16 KB | drupal_was_my_past |
#4 | role-tests-624854-4.patch | 2.04 KB | drupal_was_my_past |
editing-roles-test.patch | 2.35 KB | mcjim | |
Comments
Comment #1
lilou CreditAttribution: lilou commentedComment #3
mcjim CreditAttribution: mcjim commentedJust 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.
Comment #4
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedRe-roll and update patch from #1.
Comment #5
xjmThanks for your work on this patch. Here's a few things to fix:
The docblock summaries should begin with a third-person verb, e.g., "Tests."
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.
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.
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.
Comment #6
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedThanks @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.
Comment #8
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedRe-roll patch from #6.
Comment #9
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedComment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedcame 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.
Comment #11
drupal_was_my_past CreditAttribution: drupal_was_my_past commented@beejeebus Good catch! I'm closing this issue as a duplicate.