Several tokens are broken, especially file tokens; test coverage of every core token [#701818]

Comment	File	Size	Author
#20	701818-token-tests-D7.patch	45.74 KB	Dave Reid

#18	701818-token-tests-D7.patch	46.51 KB	Dave Reid

#13	701818_token_tests.patch	44.71 KB	mcarbone

#9	701818_token_tests.patch	44.82 KB	mcarbone

#4	701818_token_tests.patch	46.87 KB	mcarbone

#2	701818_token_tests.patch	46.42 KB	mcarbone

Comment #1

mcarbone CreditAttribution: mcarbone commented 1 February 2010 at 23:36

Issue tags:

+Needs tests

Adding tag.

Log in or register to post comments

Comment #2

mcarbone CreditAttribution: mcarbone commented 13 February 2010 at 02:02

Status:

Active

» Needs review

File	Size
701818_token_tests.patch	46.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.

Log in or register to post comments

Comment #3

13 February 2010 at 02:30

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #4

mcarbone CreditAttribution: mcarbone commented 26 February 2010 at 01:52

Status:

Needs work

» Needs review

File	Size
701818_token_tests.patch	46.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.

Log in or register to post comments

Comment #5

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 8 March 2010 at 07:05

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. :)

Log in or register to post comments

Comment #6

mcarbone CreditAttribution: mcarbone commented 8 March 2010 at 17:48

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.

Log in or register to post comments

Comment #7

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 9 March 2010 at 20:48

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

Log in or register to post comments

Comment #8

mcarbone CreditAttribution: mcarbone commented 9 March 2010 at 21:40

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."

Log in or register to post comments

Comment #9

mcarbone CreditAttribution: mcarbone commented 10 March 2010 at 02:25

Status:

Needs work

» Needs review

File	Size
701818_token_tests.patch	44.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.

Log in or register to post comments

Comment #10

mcarbone CreditAttribution: mcarbone commented 5 April 2010 at 18:28

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.

Log in or register to post comments

Comment #11

mcarbone CreditAttribution: mcarbone commented 5 April 2010 at 18:28

Issue tags:

-Needs tests

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

Log in or register to post comments

Comment #12

5 April 2010 at 19:43

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

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

Log in or register to post comments

Comment #13

mcarbone CreditAttribution: mcarbone commented 6 April 2010 at 00:35

Category:	task	» bug
Status:	Needs work	» Needs review

File	Size
701818_token_tests.patch	44.71 KB

Re-rolled to keep up with HEAD.

Log in or register to post comments

Comment #14

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 20 April 2010 at 07:57

Priority:

Normal

» Critical

Bumping to critical.

Log in or register to post comments

Comment #15

catch

he/him

English

CreditAttribution: catch commented 20 April 2010 at 08:01

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.

Log in or register to post comments

Comment #16

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 20 April 2010 at 08:14

Status:

Needs work

» Needs review

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

Log in or register to post comments

Comment #17

catch

he/him

English

CreditAttribution: catch commented 20 April 2010 at 08:15

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #18

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 20 April 2010 at 08:55

Status:

Needs work

» Needs review

File	Size
701818-token-tests-D7.patch	46.51 KB

Log in or register to post comments

Comment #19

catch

he/him

English

CreditAttribution: catch commented 20 April 2010 at 09:05

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.

Log in or register to post comments

Comment #20

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 20 April 2010 at 09:19

Status:

Needs work

» Needs review

File	Size
701818-token-tests-D7.patch	45.74 KB

Log in or register to post comments

Comment #21

catch

he/him

English

CreditAttribution: catch commented 20 April 2010 at 09:36

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.