On Dec 16, 2009, at 1:13 AM, Silviu Paragina wrote:
> Luke Kanies wrote:
>>
>> Essentially, all new tests should be in rspec, and if you're
>> modifying
>> existing behaviour, if at all possible the tests for the existing
>> behaviour should be moved to rspec.
>>
> Not really modifying behavior. (#2689 feature patch) Only switching
> to another ruby binding. So the classes should have an identical
> behavior.
>
>> We can help if you have trouble with this.
>>
>>
> In the previous patch I made I have modified the test in test/util/
> metric.rb. The problem there was that the test required a folder to
> exists (which puppet creates somewhere else, not sure where). Should
> I still include this change in the patch?
>
> Yesterday, while making the requested changes for the patch, I
> stumbled upon the rspec tests, specifically:
> in spec/unit/util/metric.rb
>
> # LAK: I'm not taking the time to develop these tests right now.
> # I expect they should actually be extracted into a separate class
> # anyway.
> it "should be able to graph metrics using RRDTool"
>
> it "should be able to create a new RRDTool database"
>
> it "should be able to store metrics into an RRDTool database"
>
>
> I think I can port all this stuff and check if anything throws an
> exception, but I can't find out if the graphed png and/or database
> is correct.
> Not sure how to write them clean, in the original test this was done
> with about 70 lines of code, which seems a lot for how rspec should
> be done.
The only thing you should worry about testing is if the right methods
are being called on the RRD library -- you have to trust that RRD is
doing the right thing.
> How should I extract it intro separate classes (naming, files, what
> should still go in the new file, what in the old)?
Really, 95% of the Metric class actually handles our interface to
RRD. It might make sense to make a 'MetricGraph' class that knows how
to talk to RRD, and then have Metric talk to it. I'd mostly be a
question of just moving the methods over and changing how the methods
are called.
I will say, though, this isn't really required. This is a simple
enough integration that, if you aren't up to all these changes, I'd be
comfortable with a testless patch as long as someone else can try the
code and confirm that it works for them.
> Last, but not least, should I do this in one commit (1 patch e-mail)
> or split it in functionality changes, test changes, documentation.
I always prefer code and its tests to be in the same commit, but then
separating different kinds of changes -- e.g., one change to move the
methods into a new class, and then another change to switch libraries.
--
Hoare's Law of Large Problems:
Inside every large problem is a small problem struggling to get
out.
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.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.
