Review Request: Framework for json tests [Attn. lang maintainers]

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107489/#review22694
-----------------------------------------------------------


Hey Olivier - this is looking really nice as usual :)

I've added some nitpicks here and there, but esp. the JSON stuff should imo be optional, i.e. only when QJSON was found. The rest should be straight forward I think.

Cheers!


tests/CMakeLists.txt
<http://git.reviewboard.kde.org/r/107489/#comment17346>

    This should be an optional include, only if qjson is found - see plugins/CMakeLists.txt . Then we should also make the usage of that code in KDevelop optional, again only if QJson is found.
    
    and maybe also only when kdevplatformjsontests is found? hmm



tests/json/CMakeLists.txt
<http://git.reviewboard.kde.org/r/107489/#comment17347>

    this should be ${qjson_LIBRARIES}



tests/json/declarationvalidator.h
<http://git.reviewboard.kde.org/r/107489/#comment17348>

    use Q_DISABLE_COPY(DeclarationValidator)



tests/json/declarationvalidator.h
<http://git.reviewboard.kde.org/r/107489/#comment17349>

    Since this is public API, it should be pimpled and the implementation hidden



tests/json/declarationvalidator.cpp
<http://git.reviewboard.kde.org/r/107489/#comment17350>

    put : on new line, break also before ,
    
    Foo::Foo()
    : bar()
    , asdf()
    {
    }



tests/json/declarationvalidator.cpp
<http://git.reviewboard.kde.org/r/107489/#comment17351>

    considering that you need to delete the parser anyways in case of an error, why not always create it on the stack?



tests/json/delayedoutput.h
<http://git.reviewboard.kde.org/r/107489/#comment17354>

    this needs an example of how this is used, esp. the nested Delay class.



tests/json/delayedoutput.h
<http://git.reviewboard.kde.org/r/107489/#comment17352>

    call it self(), following e.g. ICore::self(), DUChain::self() and the like



tests/json/delayedoutput.h
<http://git.reviewboard.kde.org/r/107489/#comment17353>

    please hide the implementation again for public API



tests/json/delayedoutput.cpp
<http://git.reviewboard.kde.org/r/107489/#comment17355>

    you can also use qPrintable(QString(output.second -1, ' '))



tests/json/jsondeclarationtests.h
<http://git.reviewboard.kde.org/r/107489/#comment17356>

    please break this line, e.g just before every .arg



tests/json/jsondeclarationtests.h
<http://git.reviewboard.kde.org/r/107489/#comment17360>

    shouldn't this be nested in the KDevelop namespace?



tests/json/jsonducontexttests.h
<http://git.reviewboard.kde.org/r/107489/#comment17361>

    again, KDevelop::DUContextTests?



tests/json/jsontesthelpers.h
<http://git.reviewboard.kde.org/r/107489/#comment17362>

    again, KDevelop::



tests/json/jsontypetests.h
<http://git.reviewboard.kde.org/r/107489/#comment17363>

    again KDevelop::



tests/json/testsuite.h
<http://git.reviewboard.kde.org/r/107489/#comment17359>

    what is this check doing? I'm bit-arithmetic impaired...



tests/json/testsuite.h
<http://git.reviewboard.kde.org/r/107489/#comment17358>

    const& the variant?



tests/json/testsuite.h
<http://git.reviewboard.kde.org/r/107489/#comment17357>

    again Q_DISABLE_COPY


- Milian Wolff


On Nov. 27, 2012, 1:41 p.m., Olivier Jean de Gaalon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107489/
> -----------------------------------------------------------
> 
> (Updated Nov. 27, 2012, 1:41 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> This patch provides a framework to allow creating json tests for arbitrary objects.
> In particular, this allows declarations to be tested using a simple DUChainVisitor (also included) which extracts json data from the declaration's comments and runs the tests specified therein.
> This can be used for any language (see kdevelop patch for cpp's implementation).
> 
> Please pay special attention to cmake stuff when reviewing because I have no idea what I'm doing.
> 
> Why not C++11?
> > We don't support it in KDevelop yet.
> 
> Why not Boost?
> > NIH
> 
> Why do the json test functions need to be included instead of linked in?
> > Because you'll add your own tests, which will be added to a different instantiation of TestSuite than the provided ones because they come from another library. C++11 might help here with the export keyword, but it's not really important.
> 
> 
> Diffs
> -----
> 
>   tests/CMakeLists.txt 4a306a4 
>   tests/json/CMakeLists.txt PRE-CREATION 
>   tests/json/declarationvalidator.h PRE-CREATION 
>   tests/json/declarationvalidator.cpp PRE-CREATION 
>   tests/json/delayedoutput.h PRE-CREATION 
>   tests/json/delayedoutput.cpp PRE-CREATION 
>   tests/json/jsondeclarationtests.h PRE-CREATION 
>   tests/json/jsonducontexttests.h PRE-CREATION 
>   tests/json/jsontesthelpers.h PRE-CREATION 
>   tests/json/jsontypetests.h PRE-CREATION 
>   tests/json/testsuite.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/107489/diff/
> 
> 
> Testing
> -------
> 
> Who tests the testers?
> 
> 
> Thanks,
> 
> Olivier Jean de Gaalon
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20121128/45743235/attachment.html>