Review Request 112100: Support for unittest python assets and some basic cleanup

[Sven Brauch]

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


Good change overall, I very much like having a better way to do this (it was crap before), but please look into the issues I found. ;)

Thanks!


duchain/tests/pyduchaintest.h
<http://git.reviewboard.kde.org/r/112100/#comment28017>

    Why are those pointers? If there's no good reason to use a pointer, don't use one ;)



duchain/tests/pyduchaintest.h
<http://git.reviewboard.kde.org/r/112100/#comment28018>

    QStack -> QList / QVector unless you really need a stack (doesn't look like it)
    Also please return the list of found files, instead of passing it by reference
    



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28019>

    use rootDir.setNameFilters(QStringList() << "*.py")



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28020>

    That looks wrong. Why are you allocating QString instances on the heap? String data is implicitly shared anyways, just use a stack object and QString will take care of the optimizations.



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28021>

    QDir* -> QDir



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28022>

    If you want to optimize, here you can use foreach ( const QString& filename, foundfiles ) { ... } ;)



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28023>

    Please use QDir::tempPath() instead of /tmp, I was doing this wrong too ;)



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28024>

    imo no, just assert. if we can't make temporary directories, we can't run tests, no point in recovering.



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28025>

    Why did you comment this? It would be nice to clean up all the files in the end. If you do that elsewhere, remove this code altogether (instead of commenting it)



duchain/tests/pyduchaintest.cpp
<http://git.reviewboard.kde.org/r/112100/#comment28026>

    ?


- Sven Brauch


On Aug. 15, 2013, 3:49 p.m., Zaar Hai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112100/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2013, 3:49 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> * Each test now can have python files in duchain/tests/data/<testFunctionName>
> * Those files will be automatically parsed before test function gets invoked
> * Temporary directory is created before test start and all TestFiles are staged there (instead of /tmp)
> 
> Sven,
> * Please have a look at my FIXME lines - I'm not sure what is the right way to abort tests execution
> * I've changed testImportDeclarations test a bit (the second data line) - please have a look that I did not do anything stupid
> 
> Thanks,
> Zaar
> 
> 
> Diffs
> -----
> 
>   duchain/tests/CMakeLists.txt 7ff16e2 
>   duchain/tests/data/testImportDeclarations/i.py PRE-CREATION 
>   duchain/tests/data/testInheritance/i.py PRE-CREATION 
>   duchain/tests/data/testMultiFromImport/i.py PRE-CREATION 
>   duchain/tests/pyduchaintest.h 5fddab4 
>   duchain/tests/pyduchaintest.cpp 00573be 
> 
> Diff: http://git.reviewboard.kde.org/r/112100/diff/
> 
> 
> Testing
> -------
> 
> make test
> 
> 
> Thanks,
> 
> Zaar Hai
> 
>

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