We need test coverage over all tokens in core, not just on a few tokens to test replacement as we currently have in system.test. For example, lack of token coverage let this bug go uncovered by testing: #609090: Invalid call to node_get_types() in node.tokens.inc

I'm currently working on these tests.

CommentFileSizeAuthor
#20 701818-token-tests-D7.patch45.74 KBdave reid
#18 701818-token-tests-D7.patch46.51 KBdave reid
#13 701818_token_tests.patch44.71 KBmcarbone
#9 701818_token_tests.patch44.82 KBmcarbone
#4 701818_token_tests.patch46.87 KBmcarbone
#2 701818_token_tests.patch46.42 KBmcarbone

Comments

mcarbone’s picture

mcarbone commented
Issue tags: +Needs tests

Adding tag.

mcarbone’s picture

mcarbone commented
Status: Active » Needs review
StatusFileSize
new 701818_token_tests.patch46.42 KB

Attached is a patch with tests for every Drupal core token. While writing the tests, I found quite a few various bugs involving token generation code, all of which are also fixed in this patch. Many of these bugs were found in the file token code, which was very outdated.

I'm setting this to 'needs review' since there's a bit to look like, although this patch will fail the testbot until #609118: node.tokens.inc: $node->body issues is resolved, and this patch is tweaked to reflect the fix.

Status: Needs review » Needs work

The last submitted patch, 701818_token_tests.patch, failed testing.

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 701818_token_tests.patch46.87 KB

Hurrah, I was able to get around waiting for #609118: node.tokens.inc: $node->body issues to be resolved by making sure to explicitly create a summary when creating the node, so these tests, if they get by the testbot, are ready to be reviewed for reals.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Needs work

Awesome job getting test coverage!

Instead of building the large array of tokens strings to compare, can we run them as individual cases?

$tests = array();
$tests['[comment:cid]'] = $comment->cid;
$tests['[comment:pid]'] = $comment->pid;
...

foreach ($tests as $input => $expected) {
  $output = token_replace($input, array('comment' => $comment));
  $this->assertEqual($output, $expected);
}

This way is going to be much easier to debug when something is going wrong. :)

mcarbone’s picture

mcarbone commented

I borrowed that approach from the already existing testTokenReplacement test in system.test, so I assumed it was OK. But I guess since we now have so many more tests like this, it's worth fixing these new ones, and also the existing system ones. Will get to that soon.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

We should probably split out the comment:changed token into its own issues as well.

mcarbone’s picture

mcarbone commented

Why? There were like 15-20 bugs that I fixed when writing these tests, and the comment:changed one doesn't seem any worse to me than the fact that the file tokens were almost all broken. If you want, we can rename this issue to something like "Fix broken tokens and add tests."

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 701818_token_tests.patch44.82 KB

Re-wrote the tests to test each token individually as suggested in #5. I left the original tests in system.test alone because they aren't there to test individual tokens, and there is value there in testing them as bigger strings.

mcarbone’s picture

mcarbone commented
Title: Test coverage of every core token » Several tokens are broken, especially file tokens; test coverage of every core token

Re-naming and requesting a re-test.

mcarbone’s picture

mcarbone commented
Issue tags: -Needs tests

#9: 701818_token_tests.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, 701818_token_tests.patch, failed testing.

mcarbone’s picture

mcarbone commented
Category: task » bug
Status: Needs work » Needs review
StatusFileSize
new 701818_token_tests.patch44.71 KB

Re-rolled to keep up with HEAD.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Priority: Normal » Critical

Bumping to critical.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

The node body fix here isn't quite complete, see #609118: node.tokens.inc: $node->body issues which I've just marked as duplicate, but which needs the same fix copying over here. That should be applied to comment body too.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

#13: 701818_token_tests.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

No way this applies since the other patch was just committed.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 701818-token-tests-D7.patch46.51 KB
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

$comment->body needs the same change as was made for $node->body in #609118: node.tokens.inc: $node->body issues, we can't guarantee that safe_value is always set.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 701818-token-tests-D7.patch45.74 KB
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Patch looks good. All the actual bug fixes are very trivial - renaming variables etc., tests are consistent and there's loads of them, comment->body issue is properly fixed now.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Well this is easily the most satisfying patch I've committed in a long time. Great work, Marco!

Committed to HEAD! :D

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.