[Re: Response to your last code review and FLUID-3116, FLUID-3117]

[Justin]

Hello,

Here are some further thoughts.

indexes
======

I also like the idea that you have good names for your indexes.  
However, I'm only worried about them being hardcoded. The danger is  
that if even the order of the modules is changed, the tests could  
fail. What I was thinking was more along the lines of writing a  
function that will find the index of the module based on its name.  
This way you won't have to manually keep track of the indexes.

data location
==========

It's probably better if the unit tests and the demo have no  
dependencies with each other. Currently the unit tests depend on demo  
for the data. What you could do is to keep the data in a separate file  
and include that into both pages. One possible solution would be to  
have a js file that  just sets that variable with the data.

- Justin


On 21-Aug-09, at 11:12 AM, Laurel A. Williams wrote:

> (reposted to the list - accidentally sent this directly to Michelle  
> and forgot to cc the list)
>
> Good to have your thoughts Michelle and I think you have solidified my
>
> feelings on this too. There may be a little more I can do to ensure  
> that the tests don't get broken as easily. For instance, I should  
> probably add a warning comment to the data, reminding coders that  
> changing the data may impact the tests. I was also thinking that the  
> constants for the module indexes might be better placed next to the  
> data itself, rather than in the tests, so that if someone adds or  
> removes a module, the index values could be changed more readily.
>
> Will await Justin's comments but am thinking that your reasoning may  
> win him over ;)
>
>
> Also, on another note, you and I chatted way back about other things  
> I should change in builder.js. I put comment notes in the file to  
> remind me (line 23) - but now I can't remember for the life of me  
> what you or I meant, and Justin wasn't sure either from a quick  
> read. So maybe we could talk about that quickly today?? I suggest if  
> you have time to check the code out and look at it briefly maybe it  
> will jog your memory and then we can talk later (maybe 2:30'ish) in  
> the channel or if you are really busy a quick email would be fine  
> too. Recall that builder.js is the "demo" code that a developer  
> would add to their web page to use the customBuild component on  
> their website. The comments you made were about moving some code  
> from builder.js into the component.
>
> Sorry for the last minute request...I only just realized I might  
> want to check in with you before you went away!
>
> Laurel
>
> michelle.dsouza at utoronto.ca wrote:
>> Hi Laurel,
>>
>> Good job on refactoring these tests. Please see my answer below.
>>
>>
>> Quoting "Laurel A. Williams" <laurel.williams at utoronto.ca>:
>>
>>
>>> As I was working with the tests, I noted that we reproduced the  
>>> exact
>>> same data in the tests. I refactored to use only one copy of the  
>>> data,
>>> but perhaps it was a case of "premature optimization".
>>>
>>> Note though, since this is a piece of demo code, the
>>> fluid.customBuild.demo.completeFluidInfusionData data does not  
>>> need to
>>> change - it could stay the same even as infusion expands, since the
>>> data for the 'live customBuilder' will always come from the
>>> build.properties file and not from this data structure. However, I'm
>>> thinking that people will be tempted to change the data as the real
>>> infusion expands, in which case they will have to change the tests  
>>> to
>>> reflect those changes...and some of the hard coding will be a pain  
>>> to
>>> change.
>>>
>>> I did try to address this somewhat by providing some constants for  
>>> the
>>> indexes of the specific modules in the data which you wrote tests
>>> around. In customBuild-tests.js you can find:
>>>
>>>       var INLINE_EDIT_INDEX = 7;
>>>       var PAGER_INDEX = 8;
>>>       var PROGRESS_INDEX = 9;
>>>       var REORDERER_INDEX = 10;
>>>       var UIOPTIONS_INDEX = 12;
>>>
>>> So, I think that outlines all of the details around this data
>>> structure...what do you think...should it simply be duplicated in  
>>> both
>>> the tests and the demo to prevent people from breaking the tests
>>> inadvertently? I'm not convinced, but honestly don't think it is
>>> critical one way or the other.
>>>
>>
>> I think it would be best to have a single copy of the demo data and  
>> have the tests run against that. It's true that if someone updates  
>> the demo data the tests may break, however, that might be a good  
>> thing. If we at some point change the structure of the data, not  
>> just the content, we would want the tests to break. It is more  
>> likely that someone will update the demo data rather then the test  
>> data in this case.
>>
>> Also, as you said, I don't feel there is any need to update the  
>> demo data when new modules are added. Unless of course the pattern  
>> of the additional module is different - a special case. In that  
>> case, we'd want the test data updated anyway because we would need  
>> to write a new test.
>>
>> As someone coming into these tests cold, I actually like the named  
>> indexes because it makes it clear to me in the test what is being  
>> tested. It's easier for me to look in the data for 'pager' rather  
>> then index 8.
>>
>> Hope this helps,
>>
>> Michelle
>>
>>
>
> < 
> laurel_williams 
> .vcf>_______________________________________________________
> fluid-work mailing list - fluid-work at fluidproject.org
> To unsubscribe, change settings or access archives,
> see http://fluidproject.org/mailman/listinfo/fluid-work