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

[Zaar Hai]

> On Aug. 15, 2013, 3:54 p.m., Sven Brauch wrote:
> > 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!

Fixed most of the issues. Please see my comments on the rest. Updated diff.
Thanks for you hints!


> On Aug. 15, 2013, 3:54 p.m., Sven Brauch wrote:
> > duchain/tests/pyduchaintest.cpp, line 794
> > <http://git.reviewboard.kde.org/r/112100/diff/1/?file=179751#file179751line794>
> >
> >     ?

Because several lines above you say:
if ( isaliased && shouldBeAliased) ...
(in that order) and you reverse the order in else statement. It just took me a few more second to get the fact that you just added "!" marks. That's why I've made order similar (does not influence anything but readability).


> On Aug. 15, 2013, 3:54 p.m., Sven Brauch wrote:
> > duchain/tests/pyduchaintest.cpp, line 174
> > <http://git.reviewboard.kde.org/r/112100/diff/1/?file=179751#file179751line174>
> >
> >     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)

Forgot to remove debug stuff. Fixed in revision two of the diff.


> On Aug. 15, 2013, 3:54 p.m., Sven Brauch wrote:
> > duchain/tests/pyduchaintest.cpp, line 141
> > <http://git.reviewboard.kde.org/r/112100/diff/1/?file=179751#file179751line141>
> >
> >     Please use QDir::tempPath() instead of /tmp, I was doing this wrong too ;)
> 
> Sven Brauch wrote:
>     Oh and please don't use character arrays. Use QString.

Well, if I do
QDir tempdirname;
tempdirname = QDir(QDir::tempPath());

Then how do I pass output of
 tempdirname.absoluteFilePath("kdev-python-test.XXXXXX")
to mkdtemp, which is a C function, and get back its result, which also can be NULL?
Also mkdtemp modifies passed character array in place.


> On Aug. 15, 2013, 3:54 p.m., Sven Brauch wrote:
> > duchain/tests/pyduchaintest.h, line 86
> > <http://git.reviewboard.kde.org/r/112100/diff/1/?file=179750#file179750line86>
> >
> >     Why are those pointers? If there's no good reason to use a pointer, don't use one ;)

Coding in C for toooo long I guess :)


- Zaar


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


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/f55b11c0/attachment-0001.html>