Patch split from: http://drupal.org/node/336697

Tests should use a descriptive message about the assertion rather than the default. Plus: There is a duplicate assertion with the same test.

Files: 
CommentFileSizeAuthor
#56 drupal-1195254-56-followup.patch811 bytesDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 37,020 pass(es).
[ View ]
#50 drupal-1195254-50.patch49.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,140 pass(es).
[ View ]
#50 interdiff.txt9.28 KBtim.plunkett
#44 taxonomy-test-cleanup-1195254-43.patch46.21 KBtrrroy
PASSED: [[SimpleTest]]: [MySQL] 39,076 pass(es).
[ View ]
#43 drupal-1195254-40.patch46.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,055 pass(es).
[ View ]
#40 taxonomy-test-cleanup-1195254-40.patch46.9 KBtrrroy
PASSED: [[SimpleTest]]: [MySQL] 39,072 pass(es).
[ View ]
#35 taxonomy-test-cleanup-1195254-35.patch46.89 KBtrrroy
FAILED: [[SimpleTest]]: [MySQL] 38,913 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#34 taxonomy-test-cleanup-1195254-34.patch46.89 KBtrrroy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/taxonomy/taxonomy.test.
[ View ]
#32 taxonomy-test-cleanup-1195254-32.patch44.07 KBtrrroy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-test-cleanup-1195254-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 drupal-1195254-27.patch49.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1195254-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 taxonomy-cleanup-1195254-23.patch49.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 36,297 pass(es).
[ View ]
#23 interdiff.txt11.41 KBxjm
#22 taxonomy-test-cleanup-1195254-22.patch49.25 KBkid_icarus
PASSED: [[SimpleTest]]: [MySQL] 36,035 pass(es).
[ View ]
#22 interdiff.txt4.66 KBkid_icarus
#19 taxonomy-test-cleanup-1195254-19.patch50.74 KBunderq
PASSED: [[SimpleTest]]: [MySQL] 35,914 pass(es).
[ View ]
#14 interdiff.txt8.34 KBunderq
#12 taxonomy-test-cleanup-1195254-12.patch54.08 KBunderq
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-test-cleanup-1195254-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 taxonomy-test-cleanup-1195254-9.patch47.49 KBkid_icarus
PASSED: [[SimpleTest]]: [MySQL] 35,921 pass(es).
[ View ]
#9 interdiff.txt836 byteskid_icarus
#7 taxonomy-test-cleanup-1195254-7.patch47.48 KBunderq
PASSED: [[SimpleTest]]: [MySQL] 35,721 pass(es).
[ View ]
#5 taxonomy-test-cleanup-1195254-5.patch13.83 KBkid_icarus
PASSED: [[SimpleTest]]: [MySQL] 35,654 pass(es).
[ View ]
#1 taxonomy-test-cleanup-1195254-1.patch14.14 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 31,429 pass(es).
[ View ]

Comments

oriol_e9g’s picture

Status:Active» Needs review
Issue tags:+Novice
StatusFileSize
new14.14 KB
PASSED: [[SimpleTest]]: [MySQL] 31,429 pass(es).
[ View ]

Tests wording improve and duplicated assertion removed (patch line 83).

xjm’s picture

Assigned:oriol_e9g» Unassigned
Status:Needs review» Needs work

This will need a reroll.

droplet’s picture

wow. I hope drupal also has change notices of such strings changes. only a DOT.. if guys working on LDO, they should re-translate all these string again...

xjm’s picture

@droplet re: #3: This issue is not tagged for backport. Also, I'm fairly certain no one translates test assertions. In fact, there's an issue somewhere recommending people don't even use t() on them. It's just a cleanup...

kid_icarus’s picture

Status:Needs work» Needs review
StatusFileSize
new13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 35,654 pass(es).
[ View ]

Re-rolled. This patch applies cleanly at commit d8a4854.

xjm’s picture

Status:Needs review» Needs work

Thanks @kid_icarus!

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -259,12 +259,12 @@ class TaxonomyVocabularyUnitTest extends TaxonomyWebTestCase {
+    $this->assertEqual(0, db_query('SELECT COUNT(*) FROM {taxonomy_term_data}')->fetchField(), t('Terms deletion when vocabulary is removed.'));

The assertion message here is a little goofy. Let's rephrase this as:
Terms are deleted when the vocabulary is deleted.

Also, while we're cleaning these lines up, let's remove t() from the assertion message texts in the lines we are already changing. Reference:
http://drupal.org/simpletest-tutorial-drupal7#t

Thanks!

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new47.48 KB
PASSED: [[SimpleTest]]: [MySQL] 35,721 pass(es).
[ View ]

Update #5 with trying to fix #6 ;)

tim.plunkett’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -722,13 +726,13 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
+    $this->assertFalse(field_info_field($field_name), t('Field %field_name does not exist.', array('%field_name' => $field_name)));

This should use format_string(), not t()

Otherwise I think this is ready to go!

kid_icarus’s picture

StatusFileSize
new836 bytes
new47.49 KB
PASSED: [[SimpleTest]]: [MySQL] 35,921 pass(es).
[ View ]

Addresses #8.

kid_icarus’s picture

Status:Needs work» Needs review
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Looks great!

underq’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new54.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-test-cleanup-1195254-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I think looks better ;)

tim.plunkett’s picture

Can you post an interdiff? I read through the patch in #9 very closely, I'd rather just see the changes made.

underq’s picture

StatusFileSize
new8.34 KB
oriol_e9g’s picture

Status:Needs review» Reviewed & tested by the community

Remove t() functions in tests assertions it's good!

tim.plunkett’s picture

RTBC seconded!

catch’s picture

Issue tags:-Novice

I missed this in the queue, and just committed taxonomy entities as classed objects which very likely breaks this. Sending for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Novice

The last submitted patch, taxonomy-test-cleanup-1195254-12.patch, failed testing.

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new50.74 KB
PASSED: [[SimpleTest]]: [MySQL] 35,914 pass(es).
[ View ]

I have reroll patch ;)

xjm’s picture

Status:Needs review» Needs work

The following formatting changes are incorrect:

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -36,7 +37,7 @@ class TaxonomyWebTestCase extends DrupalWebTestCase {
-    ));
+      ));

@@ -52,15 +53,16 @@ class TaxonomyWebTestCase extends DrupalWebTestCase {
-    ));
+      ));

@@ -980,17 +985,17 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
-    ));
+      ));

@@ -1205,8 +1212,8 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1216,13 +1223,13 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();
...
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1232,13 +1239,13 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1257,13 +1264,13 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();
...
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1274,13 +1281,13 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();
...
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1291,13 +1298,13 @@ class TaxonomyTermIndexTestCase extends TaxonomyWebTestCase {
-    ))->fetchField();
...
+      ))->fetchField();
...
-    ))->fetchField();
...
+      ))->fetchField();

@@ -1498,10 +1510,9 @@ class TaxonomyTermFieldTestCase extends TaxonomyWebTestCase {
-    }
-    catch (FieldValidationException $e) {
...
+    } catch (FieldValidationException $e) {

@@ -1509,10 +1520,9 @@ class TaxonomyTermFieldTestCase extends TaxonomyWebTestCase {
-    }
-    catch (FieldValidationException $e) {
...
+    } catch (FieldValidationException $e) {

xjm’s picture

So the current task is to reroll this patch without the above changes. Thanks!

kid_icarus’s picture

Status:Needs work» Needs review
StatusFileSize
new4.66 KB
new49.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,035 pass(es).
[ View ]

Rerolled, addressing #21.

xjm’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new11.41 KB
new49.29 KB
PASSED: [[SimpleTest]]: [MySQL] 36,297 pass(es).
[ View ]

#22 looks good. Thanks @kid_icarus.

Attached just corrects the re-indented docblocks to use standard summaries and adds periods to a few of the (already updated) assertion messages that were missed.

This is RTBC. (I only made cosmetic changes.)

xjm’s picture

Issue tags:+needs backport to D7

Also, despite my comment in #4... (I've learned a lot in the past four months.) This is an appropriate (and desirable) backport. None of these strings are actually translated in practice in D7, and it's better to keep the two codebases in line where possible.

Note that this issue should probably wait on thresholds and so might need a reroll again at some point before it goes in.

catch’s picture

#23: taxonomy-cleanup-1195254-23.patch queued for re-testing.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks folks. Committed/pushed to 8.x. It'd be good to get rid of the UnitTest that's not a unit test in there as well, but doesn't need to be done here.

Moving to 7.x for backport, I agree it's better to keep the code bases as in sync as possible - makes future backports easier.

tim.plunkett’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new49.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1195254-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

The diffstat is off because there was a whole hunk that was indented wrong in D8 that is still correct in D7.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

Didn't I RTBC this backport already? Durr.

webchick’s picture

This looks good, but i'd like to hold it until after wednesday's release so we don't inadvertently break any other patches.

Berdir’s picture

#27: drupal-1195254-27.patch queued for re-testing.

It's not going to apply anymore anyway :)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, drupal-1195254-27.patch, failed testing.

trrroy’s picture

Status:Needs work» Needs review
StatusFileSize
new44.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-test-cleanup-1195254-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

patch re-rolled

Status:Needs review» Needs work

The last submitted patch, taxonomy-test-cleanup-1195254-32.patch, failed testing.

trrroy’s picture

Status:Needs work» Needs review
StatusFileSize
new46.89 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/taxonomy/taxonomy.test.
[ View ]

trying the re-roll again

trrroy’s picture

StatusFileSize
new46.89 KB
FAILED: [[SimpleTest]]: [MySQL] 38,913 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

oops. syntax error in #34 is fixed in this one

Status:Needs review» Needs work

The last submitted patch, taxonomy-test-cleanup-1195254-35.patch, failed testing.

trrroy’s picture

It appears to me that the patch works but the test for duplicate machine name failed.

Berdir’s picture

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -76,27 +77,27 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertRaw(t('Created new vocabulary %name.', array('%name' => $edit['name'])), t('Vocabulary created successfully'));
+    $this->assertRaw(format_text('Created new vocabulary %name.', array('%name' => $edit['name'])), 'Vocabulary created successfully.');

There is no format_text() in Drupal 7....

Berdir’s picture

@trrroy: The patch in #27 is using t(), is it possible that you re-rolled the wrong one?

trrroy’s picture

StatusFileSize
new46.9 KB
PASSED: [[SimpleTest]]: [MySQL] 39,072 pass(es).
[ View ]

That was a typo for 'format_string'. Too much blood in my coffeestream. Let's try this again.

trrroy’s picture

Status:Needs work» Needs review
Berdir’s picture

Shouldn't be format_string() either IMHO. Only test assertion messages should stop using t(), it still needs to be used for the actual strings that we are comparing against.

The patch in #27 was all good except the conflict because another issue was commited.

tim.plunkett’s picture

StatusFileSize
new46.88 KB
PASSED: [[SimpleTest]]: [MySQL] 39,055 pass(es).
[ View ]

If it had been t() in the assertion message, it would be replaced with format_string(). But this is in the assertion itself, and should stay as t().

trrroy’s picture

StatusFileSize
new46.21 KB
PASSED: [[SimpleTest]]: [MySQL] 39,076 pass(es).
[ View ]

Yes, you're right. Thank for setting me straight, Berdir. Wow, messy noob attempt at reroll. I have a good feeling about this one.

tim.plunkett’s picture

Status:Needs review» Needs work

The following all need to have t() restored to the first part of the assertion. It should only be removed from the message.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -76,27 +77,27 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
-    $this->assertText(t('The machine-readable name is already in use. It must be unique.'));
+    $this->assertText('The machine-readable name is already in use. It must be unique.');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -76,27 +77,27 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
-    $this->assertText(t('The machine-readable name must contain only lowercase letters, numbers, and underscores.'));
+    $this->assertText('The machine-readable name must contain only lowercase letters, numbers, and underscores.');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -153,29 +154,29 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
-    $this->assertText(t('Created new vocabulary'), t('New vocabulary was created.'));
+    $this->assertText('Created new vocabulary', 'New vocabulary was created.');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -153,29 +154,29 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
+    $this->assertRaw(format_string('Are you sure you want to delete the vocabulary %name?', array('%name' => $vocabulary->name)), '[confirm deletion] Asks for confirmation.');
+    $this->assertText('Deleting a vocabulary will delete all the terms in it. This action cannot be undone.', '[confirm deletion] Inform that all terms will be deleted.');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -153,29 +154,29 @@ class TaxonomyVocabularyFunctionalTest extends TaxonomyWebTestCase {
-    $this->assertRaw(t('Deleted vocabulary %name.', array('%name' => $vocabulary->name)), t('Vocabulary deleted'));
+    $this->assertRaw(format_string('Deleted vocabulary %name.', array('%name' => $vocabulary->name)), 'Vocabulary deleted');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -652,19 +655,19 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
-    $this->assertRaw(t('@type %title has been created.', array('@type' => t('Basic page'), '%title' => $edit["title"])), t('The node was created successfully'));
+    $this->assertRaw(format_string('@type %title has been created.', array('@type' => t('Basic page'), '%title' => $edit["title"])), 'The node was created successfully');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -684,29 +687,29 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
-    $message = t("Taxonomy field @field_name not found.", array('@field_name' => $field_name));
+    $message = format_string("Taxonomy field @field_name not found.", array('@field_name' => $field_name));

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -1510,7 +1513,7 @@ class TaxonomyTermFieldTestCase extends TaxonomyWebTestCase {
-    $this->assertRaw(t('test_entity @id has been created.', array('@id' => $id)), t('Entity was created'));
+    $this->assertRaw(format_string('test_entity @id has been created.', array('@id' => $id)), 'Entity was created.');

.

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -1645,7 +1648,7 @@ class TaxonomyTermFieldMultipleVocabularyTestCase extends TaxonomyWebTestCase {
-    $this->assertRaw(t('test_entity @id has been created.', array('@id' => $id)), 'Entity was created.');
+    $this->assertRaw(format_string('test_entity @id has been created.', array('@id' => $id)), 'Entity was created.');

.

trrroy’s picture

Status:Needs work» Needs review

I think patch #44 fixed all those issues in 45.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

My #45 was a crosspost, #44 is RTBC.

xjm’s picture

Status:Reviewed & tested by the community» Needs work

Er. The original patch at the beginning of this issue was to add periods to the assertion messages that were missing them, among other things. The periods keep disappearing in the rerolls.

xjm’s picture

Title:Taxonomy test cleanup and wording improve» Taxonomy test cleanup
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new9.28 KB
new49.85 KB
PASSED: [[SimpleTest]]: [MySQL] 39,140 pass(es).
[ View ]

Right. Got those.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Looked over this again and compared it with the patch from #27. Looks good to me.

ZenDoodles’s picture

#50: drupal-1195254-50.patch queued for re-testing.

David_Rothstein’s picture

#50: drupal-1195254-50.patch queued for re-testing.

David_Rothstein’s picture

(Patch looks good; just want to make sure it still passes tests before committing.)

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new811 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,020 pass(es).
[ View ]

But actually, can we please fix this indentation bug (which the patch introduced) because it's driving me crazy :)

I think it's OK to RTBC my own D8 patch here.

webchick’s picture

Version:8.x-dev» 7.x-dev

Committed and pushed #56 to 8.x. :)

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed

I'm going to count that as an RTBC for Drupal 7 also :)

Committed the followup there too. http://drupalcode.org/project/drupal.git/commit/f7d5d80

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