BUG: 141564
Details
Details
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
The bug report is about checking whether it's writable, you're checking the protocol here, and file will always "support writing". This patch could still make sense, though.
| src/core/copyjob.cpp | ||
|---|---|---|
| 864 | Not sure ERR_CYCLIC_COPY is the correct error for this? | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 862 | Wrong mix of URLs and paths. How can this ever work? QFileInfo takes local paths, and you're giving it a full URL? All this should be inside if (m_currentDestURL.isLocalFile()) and use m_currentDestURL.toLocalFile() as the path to give to QFileInfo. | |
| 863 | Why the "copy or move" test? The only alternative is creating a symlink, which also requires being able to write, no? | |
| 864 | It doesn't sound like it, no. This should rather be ERR_CANNOT_WRITE. | |
| 865 | This used to be an error, now it gets degraded to a warning. This means applications performing the copy will think it actually worked, only the user got (maybe) a warning.... I think this should be an error. And there should be a unittest for it. JobTest::copyFolderWithUnaccessibleSubfolder shows how to make a folder non-writable (and still be able to clean it up at the end). A similar test should be added where the destination is the one that is unaccessible (or just unwritable). | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 868–872 | emitresult() emits the corresponding signal and then commits suicide, hence no such chance possible. | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 868–872 | Still, don't call statNextSrc() then. | |
Comment Actions
@dfaure do you need tests for all three modes ie. copy,move and link
And individually for files and folders
Comment Actions
The logic is the same in all cases, the mode doesn't really make a difference, so I'm ok with picking one -- let's say Move, so we can also check that nothing disappeared.
File or folder also makes no difference, so I'm ok with just one of these.
The one thing that does make a difference is having more source items to copy/move, as discussed here, so maybe the test could try moving two files or two folders.
This comment was removed by shubham.
Comment Actions
@dfaure This is one unit test I have made by doing some modifications. would you like to give me some help?
| autotests/jobtest.cpp | ||
|---|---|---|
| 1004 | At runtime, Qt says: QSignalSpy: Not a valid signal: '' Indeed this is the getter, not a signal. result() is the signal, but you don't really need to connect to it anyway. | |
| 1007 | FAIL! : JobTest::moveDirectoryToInaccessibleFilesystem() Compared values are not the same Actual (job->error()) : 115 Expected ((int)KIO::ERR_CANNOT_WRITE): 129 115 is ERR_ACCESS_DENIED, see grep -w 15 src/core/global.h I think that's because you went too far (due to copy/pasting). The dest dir shouldn't be inaccessible, it should only be readonly. Also you forgot the cleanup at the end of the method, which breaks running the test multiple times. OK, even with all this the test doesn't pass. Changing dst_dir to use otherTmpDir() ... ok, the error code changes, now it's 137, i.e. ERR_COULD_NOT_MKDIR :-) | |
Comment Actions
In fact this is the wrong location for this code. The checks on the dest dir are in CopyJobPrivate::slotResultStating. I'm working on a different fix.