cmp_document branch

[moregan]

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.

[adamkennedy]

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" notifications@github.com 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:

#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/cmp_document branch #49/files#diff-0(5)
• M t/07_token.thttps://github.com/cmp_document branch #49/files#diff-1(37)
• M t/08_regression.thttps://github.com/cmp_document branch #49/files#diff-2(5)
• M t/19_selftesting.thttps://github.com/cmp_document branch #49/files#diff-3(3)
• M t/25_increment.thttps://github.com/cmp_document branch #49/files#diff-4(5)
• M t/26_bom.thttps://github.com/cmp_document branch #49/files#diff-5(5)
• A t/lib/PPI/Test.pmhttps://github.com/cmp_document branch #49/files#diff-6(341)
• A t/lib/PPI/Test/Object.pmhttps://github.com/cmp_document branch #49/files#diff-7(190)
• R t/lib/PPI/Test/Run.pmhttps://github.com/cmp_document branch #49/files#diff-8(185)
• A t/lib/PPI/Test/pragmas.pmhttps://github.com/cmp_document branch #49/files#diff-9(32)
• M t/ppi_token_quote_double.thttps://github.com/cmp_document branch #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//pull/49
.

[moregan]

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 ( 3353672#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";'

[wchristian]

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.

[adamkennedy]

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" notifications@github.com 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//pull/49#issuecomment-36883244
.