Re: Review Request 122755: Add testcases for KIO::del()

2015-03-07 Thread Martin Blumenstingl 


> On March 7, 2015, 10:22 a.m., Emmanuel Pescosta wrote:
> > autotests/deletejobtest.cpp, line 62
> > 
> >
> > This has broken the KIO build 
> > (http://build.kde.org/job/kio_stable_qt5/123/console)
> > 
> > QSignalSpy::QSignalSpy(const QObject * object, PointerToMemberFunction 
> > signal) is available since Qt 5.4 but KIO uses Qt 5.2
> > 
> > Please use the old QSignalSpy::QSignalSpy(const QObject * object, const 
> > char * signal) instead
> > 
> > This applies for all other QSignalSpy usages in this test
> 
> David Faure wrote:
> Fixed.

Thank you both for tracking down this issue and for fixing it!


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review77157
---


On March 6, 2015, 9:57 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 6, 2015, 9:57 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-07 Thread David Faure 


> On March 7, 2015, 10:22 a.m., Emmanuel Pescosta wrote:
> > autotests/deletejobtest.cpp, line 62
> > 
> >
> > This has broken the KIO build 
> > (http://build.kde.org/job/kio_stable_qt5/123/console)
> > 
> > QSignalSpy::QSignalSpy(const QObject * object, PointerToMemberFunction 
> > signal) is available since Qt 5.4 but KIO uses Qt 5.2
> > 
> > Please use the old QSignalSpy::QSignalSpy(const QObject * object, const 
> > char * signal) instead
> > 
> > This applies for all other QSignalSpy usages in this test

Fixed.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review77157
---


On March 6, 2015, 9:57 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 6, 2015, 9:57 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-07 Thread Emmanuel Pescosta 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review77157
---



autotests/deletejobtest.cpp


This has broken the KIO build 
(http://build.kde.org/job/kio_stable_qt5/123/console)

QSignalSpy::QSignalSpy(const QObject * object, PointerToMemberFunction 
signal) is available since Qt 5.4 but KIO uses Qt 5.2

Please use the old QSignalSpy::QSignalSpy(const QObject * object, const 
char * signal) instead

This applies for all other QSignalSpy usages in this test


- Emmanuel Pescosta


On March 6, 2015, 10:57 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 6, 2015, 10:57 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-06 Thread Martin Blumenstingl 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/
---

(Updated March 6, 2015, 9:57 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kio


Description
---

This adds some automated testcases for KIO::del().

My own code was buggy but I didn't notice this until I wrote those tests.
Maybe you want to have them upstream to make sure noone breaks KIO::del.


Diffs
-

  autotests/deletejobtest.cpp PRE-CREATION 
  autotests/CMakeLists.txt f613c1a 
  autotests/deletejobtest.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/122755/diff/


Testing
---

Tests are passing


Thanks,

Martin Blumenstingl

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-01 Thread Martin Blumenstingl 


> On March 1, 2015, 2:34 a.m., Aleix Pol Gonzalez wrote:
> > autotests/deletejobtest.cpp, line 43
> > 
> >
> > Would it be better to use the unicode escaping? This can look weird in 
> > some set ups?

I also did the same for the chinese test.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review76790
---


On March 1, 2015, 12:21 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 1, 2015, 12:21 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-01 Thread Martin Blumenstingl 


> On Feb. 28, 2015, 7:24 p.m., Emmanuel Pescosta wrote:
> > autotests/deldirtest.cpp, line 51
> > 
> >
> > KJob::NoError instead of 0?

We need a cast for that check. Maybe you can look at my solution?


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review76769
---


On March 1, 2015, 12:21 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 1, 2015, 12:21 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-01 Thread Martin Blumenstingl 


> On March 1, 2015, 2:34 a.m., Aleix Pol Gonzalez wrote:
> > Looks good to me, can you push yourself?

I can push it myself if someone decides whether that 
static_cast(KJob::NoError) is fine or if 0 should be hardcoded (like in my 
first patch).
I am fine with both - so I'd like to stick to what the "KDE way" is :)


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review76790
---


On March 1, 2015, 12:21 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated March 1, 2015, 12:21 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/deletejobtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-03-01 Thread Martin Blumenstingl 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/
---

(Updated March 1, 2015, 12:21 p.m.)


Review request for KDE Frameworks.


Changes
---

* Use unicode escaping for the German umlaut and Chinese character testcases
* Use KJob::NoError instead of a hardcoded 0


Repository: kio


Description
---

This adds some automated testcases for KIO::del().

My own code was buggy but I didn't notice this until I wrote those tests.
Maybe you want to have them upstream to make sure noone breaks KIO::del.


Diffs (updated)
-

  autotests/deletejobtest.cpp PRE-CREATION 
  autotests/CMakeLists.txt f613c1a 
  autotests/deletejobtest.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/122755/diff/


Testing
---

Tests are passing


Thanks,

Martin Blumenstingl

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-02-28 Thread Aleix Pol Gonzalez 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review76790
---

Ship it!


Looks good to me, can you push yourself?


autotests/deletejobtest.cpp


Would it be better to use the unicode escaping? This can look weird in some 
set ups?


- Aleix Pol Gonzalez


On Feb. 28, 2015, 10:21 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated Feb. 28, 2015, 10:21 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deletejobtest.h PRE-CREATION 
>   autotests/deletejobtest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-02-28 Thread Martin Blumenstingl 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/
---

(Updated Feb. 28, 2015, 9:21 p.m.)


Review request for KDE Frameworks.


Changes
---

Fixed issues noted by Emmanuel.


Repository: kio


Description
---

This adds some automated testcases for KIO::del().

My own code was buggy but I didn't notice this until I wrote those tests.
Maybe you want to have them upstream to make sure noone breaks KIO::del.


Diffs (updated)
-

  autotests/CMakeLists.txt f613c1a 
  autotests/deletejobtest.h PRE-CREATION 
  autotests/deletejobtest.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/122755/diff/


Testing
---

Tests are passing


Thanks,

Martin Blumenstingl

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122755: Add testcases for KIO::del()

2015-02-28 Thread Emmanuel Pescosta 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122755/#review76769
---


Thanks for the test! :)

Maybe you can add some negative test cases (like read-only files - have a look 
at pastetest.cpp) as well and check if KIO sets the correct error codes?
That would be really awesome!

(Another test case which can be added to this test is in 
https://bugs.kde.org/show_bug.cgi?id=340950)

Please note that I'm not a KIO developer, so better wait for an ack from a KIO 
dev before you implement my suggestions. ;)


autotests/deldirtest.h


Maybe name it DeleteJobTest instead of DelDirTest? (because deleting of 
files and folders gets tested, so DelDir isn't correct)



autotests/deldirtest.h


can be a const method



autotests/deldirtest.cpp


I would add a "number of files" parameter to createEmptyTestFiles instead 
of using a global definintion or constant.



autotests/deldirtest.cpp


KJob::NoError instead of 0?


- Emmanuel Pescosta


On Feb. 28, 2015, 4:28 p.m., Martin Blumenstingl wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122755/
> ---
> 
> (Updated Feb. 28, 2015, 4:28 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> This adds some automated testcases for KIO::del().
> 
> My own code was buggy but I didn't notice this until I wrote those tests.
> Maybe you want to have them upstream to make sure noone breaks KIO::del.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt f613c1a 
>   autotests/deldirtest.h PRE-CREATION 
>   autotests/deldirtest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/122755/diff/
> 
> 
> Testing
> ---
> 
> Tests are passing
> 
> 
> Thanks,
> 
> Martin Blumenstingl
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel