Review Request 129275: StoredTransferJob: add failing test case

[David Faure]

> On Oct. 29, 2016, 7:49 p.m., David Faure wrote:
> > autotests/jobtest.cpp, line 283
> > <https://git.reviewboard.kde.org/r/129275/diff/2/?file=483171#file483171line283>
> >
> >     It's stream.flush() that you need, to tell QTextStream to do the actual writing to the underlying device.
> >     
> >     QTextStream only flushes automatically when being deleted or when the underlying device is a file and is closed.
> >     
> >     Once doing stream.flush() here, this test fails like the other one, so this isn't related to QXmlStreamWriter, but to passing an open QTemporaryFile as the device to storedPut. We need to at least rewind it to pos 0...
> 
> David Faure wrote:
>     Confirmed, tempFile.seek(0) before KIO::storedPut() fixes it.
> 
> Elvis Angelaccio wrote:
>     So, it this something that the job should do or the clients should do before calling KIO::storedPut()?

IMHO it's something the clients should do. When you pass a QIODevice to a method, you expect the method to read from the current position. If you're being clever and you're reusing the same QIODevice for writing and for reading, then you have to rewind your iodevice. Let's see if there's a precedent in Qt... Well, yeah, QDataStream(QIODevice *) for instance, it does not seek(0) automatically, there are many cases where you wouldn't want that. In fact I can find more examples of that, and none where an implicit seek(0) happens.

Maybe you would find it more intuitive to close and reopen the device, that would work too, AFAICS.
seek(0) only works here because QTemporaryFile uses ReadWrite mode. If you were using QFile, and you had a bit of code to write and then a bit of code to re-read what you wrote, then you'd have to either:
- use ReadWrite and seek(0)
- use WriteOnly, close, reopen ReadOnly.
So it's the same here with QTemporaryFile, you need one of those two options.


- David


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


On Oct. 30, 2016, 10:20 a.m., Elvis Angelaccio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129275/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2016, 10:20 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> https://bugs.kde.org/show_bug.cgi?id=368314 seems to be an actual bug in KIO, rather than a regression in kwalletmanager. I reproduced the bug in a test case: basically `StoredTransferJob` fails to copy a QTemporaryFile when the file has been written by a QXmlStreamWriter (destination file is created, but empty).
> 
> If we write to the temp file using a QTextStreamer instead, everything works as expected...
> 
> 
> Diffs
> -----
> 
>   autotests/jobtest.h 5687b4a448d26d703d33603e5a0939bc4f8c77b4 
>   autotests/jobtest.cpp 11bac4d51e97840181aff99fdddb92cdf4db1ba2 
> 
> Diff: https://git.reviewboard.kde.org/r/129275/diff/
> 
> 
> Testing
> -------
> 
> jobtest still passes (with one expected failure).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161030/5f8d5dfb/attachment-0001.html>