Summary: | Some tests in Upload.t are not testing the right thing | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Test Suite | Assignee: | Chris Cormack <chris> |
Status: | NEW --- | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | m.de.rooy |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Bug Depends on: | 17501 | ||
Bug Blocks: | 22508 | ||
Attachments: | Bug 22644: Highlight the problem |
Description
Jonathan Druart
2019-04-05 00:59:17 UTC
Created attachment 87429 [details] [review] Bug 22644: Highlight the problem There is not a problem here, just a misunderstanding. As noted in the commment, the file does not exist here since a record is added via TestBuilder. In the preceding code you see a test for a warning about the missing file. Deleting a record twice while keeping the missing file is kind of exaggerated testing ;) So the patch here does not make sense. 162 my $delete = Koha::UploadedFiles->search({ id => $id })->delete; 163 isnt( $delete, "0E0", 'Delete successful' ); 164 isnt( -e $path, 1, 'File no longer found after delete' ); We are testing that the file has been deleted, but it never existed. Those tests do not test the whole purpose of the uploaded files. Moreover the new tests I have tried to write for bug 22508 are failing, because of this behavior. So, IMO, there is a real need here. Marcel, what do you think? How could I provide tests for bug 22508 without having this being fixed? Any ideas? (In reply to Jonathan Druart from comment #4) > Marcel, what do you think? How could I provide tests for bug 22508 without > having this being fixed? Any ideas? Will have a look very soon. The test reflects the history of Upload to Uploader and UploadedFile(s) with Koha::Object(s). I think we should put the Uploader stuff into its own test Uploader.t. And put all Koha objects related tests to UploadedFile.t. What UploadedFile::delete may need, is a return code: deleting the record may have failed, but deleting the file as well. The test could be improved by adding some temporary files. And we need some decisions on 22508 as well. See my older comment there. Especially, do we copy the file or keep just one and allow direct access to selected subdirs of the clone upload folder? |