Skip to content

File download hang fix #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 3, 2013
Merged

File download hang fix #30

merged 3 commits into from
May 3, 2013

Conversation

krnowak
Copy link
Contributor

@krnowak krnowak commented Apr 29, 2013

Fixes SOAP-77.

krnowak added 2 commits April 29, 2013 16:01
There already was a test doing it, but it was rather concentrating on
authentication, so I renamed it to testFileDownloadAuth. The new test
checks how the server behaves when downloading nonexistent or files
with no read permissions.
@dfaure-kdab
Copy link
Member

A fix with a unittest - great!

Just two comments:

  1. the file_download.txt could be created once, by doing it in the _data method, rather than doing it every time the testFileDownload() method is called.
  2. rather than a bool "expectedSuccess", it should be the actual expected error() value. It would allow a more precise QCOMPARE instead of the QVERIFY((int)reply->error() != (int)QNetworkReply::NoError);

Thanks.

File download handling is also modified to return either 404 or 403
when file is not found or couldn't be opened for reading. This commit
makes ServerTest::testGetShouldFail test failing.
@krnowak
Copy link
Contributor Author

krnowak commented Apr 30, 2013

Ad 1) It was mostly a copy of the old testFileDownload (now renamed to testFileDownloadAuth). Also, I don't see a way to call per-test cleanup. There is only general init/cleanup (called before/after every test) and similar for entire test case.

Ad 2) I made it a bool, because I would expect that "readable" case returns NoError, "nonexistent" - ContentNotFoundError and "unreadable" - ContentOperationNotPermittedError as per http://qt-project.org/doc/qt-5.0/qtnetwork/qnetworkreply.html#NetworkError-enum

But for now the code in KDSoapServerSocket just returns UnknownContentError for last two. I could, for example, modify file download handling that if it gets NULL QIODevice then it returns 404 or if it gets it but cannot open for reading then it returns 403 (see new commit anyway). But that makes yet another test case failure. Not sure how to proceed here.

dfaure-kdab added a commit that referenced this pull request May 3, 2013
Improve error handling when downloading files (not found, or not readable). Fixes SOAP-77.
@dfaure-kdab dfaure-kdab merged commit a098b35 into KDAB:master May 3, 2013
@krnowak krnowak deleted the file-download-hang-fix branch May 3, 2013 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants