PPI cmp_document branch

Reorganize t/lib modules so that names to not conflict with lib/* modules under use lib 't/lib'. Add t/lib/PPI/Test/pragmas.pm to centralize test file boilerplate. Add cmp_document family of test functions. As a sample, converted ppi_token_quote_double.t to use new test functions.

Mar 06 '14 04:03 moregan

I agree with the intent of this, but it doesn't seem to make things significantly simpler, just replaced code with similar amounts of data structs.

It does seem to make a few of the examples harder to run the test in the perl debugger.

I'm more ambivalent on losing the t::lib namespace, but it was an extremely elegant way to avoid clashes in the ppi namespace, and potential problems manipulating @inc at the same time. On Mar 5, 2014 8:18 PM, "moregan" [email protected] wrote:

Reorganize t/lib modules so that names to not conflict with lib/* modules under use lib 't/lib'. Add t/lib/PPI/Test/pragmas.pm to centralize test file boilerplate. Add cmp_document family of test functions.

As a sample, converted ppi_token_quote_double.t to use new test functions.

You can merge this Pull Request by running

git pull https://github.com/moregan/PPI cmp_document

Or view, comment on, or merge it at:

https://github.com/adamkennedy/PPI/pull/49 Commit Summary

add cmp_document family of test functions ; reorg t/lib modules

convert t/ppi_token_quote_double.t to using PPI::Test helpers

File Changes

M t/05_lexer.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-0(5)

M t/07_token.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-1(37)

M t/08_regression.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-2(5)

M t/19_selftesting.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-3(3)

M t/25_increment.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-4(5)

M t/26_bom.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-5(5)

A t/lib/PPI/Test.pmhttps://github.com/adamkennedy/PPI/pull/49/files#diff-6(341)

A t/lib/PPI/Test/Object.pmhttps://github.com/adamkennedy/PPI/pull/49/files#diff-7(190)

R t/lib/PPI/Test/Run.pmhttps://github.com/adamkennedy/PPI/pull/49/files#diff-8(185)

A t/lib/PPI/Test/pragmas.pmhttps://github.com/adamkennedy/PPI/pull/49/files#diff-9(32)

M t/ppi_token_quote_double.thttps://github.com/adamkennedy/PPI/pull/49/files#diff-10(98)

Patch Links:

https://github.com/adamkennedy/PPI/pull/49.patch

https://github.com/adamkennedy/PPI/pull/49.diff

Reply to this email directly or view it on GitHubhttps://github.com/adamkennedy/PPI/pull/49 .

Mar 06 '14 06:03 adamkennedy

Losing t::lib was based on other feedback. I'm not wedded to it myself.

I can see your point about the debugger. That's a blind spot for me.

In the cases where only one thing is being checked, tests with cmp_element will look similar, yes. My feeling is that with cmp_*:

it's easier to check more than one datapoint, and that it's more likely that extra verification will get done. E.g.: the final interpolations test checks ->content as well.
attention is drawn to the element as a whole (or elements, of course), and that it's less likely that something important flies by unnoticed. For instance, in recent months there was a change ( https://github.com/adamkennedy/PPI/commit/33536720901b81b0eb54d5533c327354735aab95#diff-96bb92d687e27596386c714a3263f2c6R46 ) to add PPI support for operators like ||= and &&=. There was an existing test to parse '$foo ||= { One => 1 };', making sure that schild(3) of the statement was a Constructor. When ||= started parsing as a single operator rather than || followed by a separate = operator, that check had to be directed at schild(2) instead. I think it would have been more obvious that ||= wasn't parsing right.
it's easier to test things jointly. When checking that "y"x3 parses as the x operator without missing anything, it's nice to be able to just write:

[
    { class => 'PPI::Token::Quote::Double', content => '"y"' },
    { class => 'PPI::Token::Operator', content => 'x' },
    { class => 'PPI::Token::Number', content => '3' },
]                        ],

Being able to ignore whitespace elements while doing the above is nice, too. Testing "y" x 3 can use the same array above and cmp_selement as opposed to cmp_element.

when many tokens are in play, the test failure message is useful:

    #   Failed test '[3]: class matches'
    #   at t/lib/PPI/Test.pm line 137.
    #          got: 'PPI::Token::Quote::Double'
    #     expected: 'PPI::Token::Quote::Single'
    #
    #     [0] PPI::Document               { class => PPI::Document }
    #     [1] PPI::Statement              { isa => PPI::Statement }
    #     [2] PPI::Token::Word            { content => print, class => PPI::Token::Word }
    # >>> [3] PPI::Token::Quote::Double   { content => "foo", class => PPI::Token::Quote::Single } <<<
    #     [4] PPI::Token::Structure       { content => ;, class => PPI::Token::Structure }
    # Looks like you failed 1 test of 10.

#   Failed test 'cmp_document: print "foo";'

Mar 06 '14 07:03 moregan

Regarding t::lib, that was my idea. While it can be nice, i didn't see any advantages by being clever in that way, when PPI::Test does the same thing and is quite explicit about what happens. On the other hand, i'm not wedded to the idea of removing that and don't mind if reverting that makes you happier, @adamkennedy. :)

Now, as for the main part of the commit:

I do think @moregan is on the right trail, but hasn't gone far enough yet. I also agree with the debuggerability complaint, as i've run into that before. I added two commits on his branch as a proof of concept to demonstrate how it could be made both more concise, easier to read, write, and still highly breakpointable:

https://github.com/adamkennedy/PPI/commits/cmp_document

In short, the main idea is: Hashes are cool, but if you end up typing the same hash again and again and again, it's better to have an array that gets converted to a hash.

Mar 06 '14 12:03 wchristian

I've been trying to remember why the hell I created t::lib in the forest place. I think it may have had something to do with passing tests in taint mode, or some other situation in which you are forbidden to modify @inc?

Adam On Mar 6, 2014 4:29 AM, "Christian Walde" [email protected] wrote:

Regarding t::lib, that was my idea. While it can be nice, i didn't see any advantages by being clever in that way, when PPI::Test does the same thing and is quite explicit about what happens. On the other hand, i'm not wedded to the idea of removing that and don't mind if reverting that makes you happier, @adamkennedy https://github.com/adamkennedy. :)

Now, as for the main part of the commit:

I do think moregan is on the right trail, but hasn't gone far enough yet. I also agree with the debuggerability complaint, as i've run into that before. I added two commits on his branch as a proof of concept to demonstrate how it could be made both more concise, easier to read, write, and still highly breakpointable:

https://github.com/adamkennedy/PPI/commits/cmp_document

Reply to this email directly or view it on GitHubhttps://github.com/adamkennedy/PPI/pull/49#issuecomment-36883244 .

Mar 08 '14 15:03 adamkennedy