On 16/09/08 21:37, Luke Kanies wrote:
> On Sep 16, 2008, at 1:56 PM, Brice Figureau wrote:
>>>
>>> I'm not opposed to this, and can see why people are interested in
>>> this.
>>>
>>> Just to be clear, this works by basically making a new variable in
>>> the
>>> current scope that is a concatenation of the specified variable and
>>> the new text/array, right?
>> Not a new variable, it adds in the current scope a variable with the
>> same name as the upper scope variable but whose value is concatenated
>> with the current rvalue.
>>
>> The implementation in the patch above allows to append _in the same
>> scope_, but I don't think this is right, hence my aforementioned
>> comment
>> that I can easily explicitely allow append operator only on new
>> descendant context.
>
> Sorry, i didn't reply to that earlier -- I, too, think appending
> should only be allowed in a new scope, not in the same scope.
OK, next version will behave like that.
>>> If so, then that seems perfectly reasonable to me, since it's very
>>> similar to just using something like:
>>>
>>> $othervar = "$var new text"
>>>
>>> It only affects lower scopes, rather than modifying the original
>>> value, so it should work great.
>> Yes.
>>
>>> As to tests, there should be sufficient lexer tests, but I agree that
>>> we don't have parser tests to speak of; all I've got are integration
>>> tests in the form of snippets, like you already have done. It might
>>> be a good time to create that parser unit test file and add tests for
>>> this code. :)
>> My problem is that I don't even know how to start a parser unit
>> rspec :-(
>>
>> The only thing I can think of is: parse some puppet snippets (or
>> should
>> I feed lexer results?), then mock the AST::XYZ object to check that
>> they
>> are called fine. Is that the right thing to do?
>>
>> Right now, I have a rspec append test, that is more an integration
>> test,
>> as it feeds puppet snippets to a parser, then compile the AST tree,
>> then
>> finally checks that the scope contains the right value, or that right
>> scope function setvar is called. Is that something you might include?
>
> Well, there are a couple of ways you can do this, but what I'd
> generally recommend is splitting all of the tests into the different
> classes.
>
> You're changing three things: an AST::VarDef class, the scope class,
> and the Parser class. I'd normally add tests for each of these three
> classes.
>
> The AST and scope classes are straightforward to test, but ask for
> more detail if you'd like help.
That makes more sense than what I did.
> The Parser class is a bit harder to test, but really, all you need to
> test is that a given chunk of text results in the appropriate AST
> instances being created. E.g., something like (this code won't work,
> but it's off the top of my head):
>
> parser = Parser.new
> AST::VarDef.expects(:new).with { |scope, otherstuff, append| append ==
> true }
> parser.parse("$var += something")
>
> In other words, you just mock that the right AST instances are being
> created with the right options.
OK, that's easier than what I first thought. I'll spread the tests in
different unit tests.
Hopefully that'll start a parser test (or you want a different patch?),
that we can later on increase with more tests.
> I will say that this stuff isn't necessarily easy to test, and I'm
> totally behind any refactoring that might make sense to make it easier.
>
>>> I think the major changes, though, are in the scope class, and it's
>>> also only tested in test/ rather than spec/.
>> OK, so I can also start a rspec scope that at least check the setvar
>> method.
>>
>> On a different side, there are demand for more complex if test (ie
>> including or, and, ==, !=, etc ala collection expressions). I'm
>> volunteer to implement this (and even already started). Is this
>> something that you can include, or should I use my spare time for
>> something else?
>
>
> I'd include them if they were available, yeah, so go ahead and do
> them, as long as you're doing tests at the same time.
Unfortunately, I'm pretty old school regarding programming (too much
time in this business I guess :-P ), so I still don't have the reflex to
start by the test part before the code part, so I always feel
implementing the test _after_ the code is a duplicated effort :-(
I guess it's time to change how I see things :-) and start to really
embrace test driven development.
> I'd *much* prefer to help you get the tests done right than worry
> about whether I'd accept your code later because maybe the tests
> aren't sufficient later, btw. You seem like quite the self-starter,
> so I'm trying to point you in the right direction and let you kick
> ass, but if you want help, just let me know and I'll do what I can.
OK, thanks, I appreciate the pointers. I think I'm now on the right
track :-)
I'll post the various patches for review on the list.
--
Brice Figureau
Days of Wonder
http://www.daysofwonder.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
