On thing I've found running tests is that the common practice of calling all the test cases in module Foo things like 'Foo access tests', 'Foo admin UI' and so on is a total pain because it makes it harder to scan down the list of tests in the UI. You've already expanded the 'Foo' item in the list of tests, so you know that it's for Foo module.

Therefore, we should change the 'name' properties in getInfo() in our test cases to start with words that distinguish them, where it makes sense to do so.

Eg, 'Flag admin tests' -> 'Admin UI'

(the word 'test' is redundant here too!)

CommentFileSizeAuthor
#3 flag-changed_names_of_test_cases-2210061-3.patch3.04 KBcs_shadow
#3 interdiff.txt899 bytescs_shadow
#1 flag-changed_names_of_test_cases-2210061-1.patch3 KBcs_shadow

Comments

cs_shadow’s picture

cs_shadow commented
Status: Active » Needs review
StatusFileSize
new flag-changed_names_of_test_cases-2210061-1.patch3 KB

Changed the name of test cases. Removed 'Flag' prefix and 'Tests' suffix. Did some other improvements to the names wherever the new names made better sense. Any suggestions are welcome.

joachim’s picture

joachim commented

Thanks for the patch!

Looks pretty good.

A few things that I'll investigate when I have more time (though feel free to beat me to it if you like) --

  1. +++ b/tests/flag.test
@@ -43,7 +43,7 @@ class FlagFlaggingCRUDTestCase extends FlagTestCaseBase {
-      'name' => 'Flagging CRUD',
+      'name' => 'CRUD UI',

    Is that definitely the UI rather than just CRUD API? 

  2. +++ b/tests/flag.test
@@ -385,7 +385,7 @@ class FlagAccessFormTestCase extends FlagTestCaseBase {
-      'name' => 'Flag access: entity forms',
+      'name' => 'Entity forms',

    We've lost that this is about access.

    'Access to flags on entity forms' maybe? 

  3. +++ b/tests/flag.test
@@ -636,7 +636,7 @@ class FlagAccessLinkTestCase extends FlagTestCaseBase {
   public static function getInfo() {
     return array(
-      'name' => 'Flag access tests',
+      'name' => 'Access UI',
       'description' => 'Access to flag and unflag entities using the basic link.',
       'group' => 'Flag',
     );
@@ -762,7 +762,7 @@ class FlagLinkTypeConfirmTestCase extends DrupalWebTestCase {

@@ -762,7 +762,7 @@ class FlagLinkTypeConfirmTestCase extends DrupalWebTestCase {
    */
   public static function getInfo() {
     return array(
-      'name' => 'Flag confirm link tests',
+      'name' => 'Confirm form',
       'description' => 'Flag confirm form link type.',
       'group' => 'Flag',
     );

    I'm not entirely sure about these two tests, as their class names don't match either, but are they a pair that test the basic link and the confirm form?

cs_shadow’s picture

cs_shadow commented
StatusFileSize
new interdiff.txt899 bytes
new flag-changed_names_of_test_cases-2210061-3.patch3.04 KB

Reg. 1, I read the tests, it seems to me that its not for UI and just tests the API, so changed it to 'CRUD API'.

@@ -43,7 +43,7 @@ class FlagFlaggingCRUDTestCase extends FlagTestCaseBase {
-      'name' => 'Flagging CRUD',
+      'name' => 'CRUD API',

Reg. 2, changed the name to 'Access to flags via entity forms'.

@@ -385,7 +385,7 @@ class FlagAccessFormTestCase extends FlagTestCaseBase {
-      'name' => 'Flag access: entity forms',
+      'name' => 'Access to flags via entity forms',

Reg.3, I read the tests and as far as I can understand, they are a pair that test the basic link and the confirm form. Though I renamed the first one to incorporate that the access is through basic links.

@@ -636,7 +636,7 @@ class FlagAccessLinkTestCase extends FlagTestCaseBase {
-      'name' => 'Flag access tests',
+      'name' => 'Access to flags via basic link',

Have a look at these changes, and let me know if in case you want any changes.
I'm attaching the new patch and its inter-diff with the previous patch for reference.

joachim’s picture

joachim commented
Status: Needs review » Fixed

Ah I see what you mean -- FlagLinkTypeConfirmTestCase doesn't bother checking access, just that the links works.

Committed. Thanks!

  • Commit 446f883 on 7.x-3.x authored by cs_shadow, committed by joachim: 
    Issue #2210061 by cs_shadow: Changed UI labels of test cases to remove '...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.