Bug 22644 - Some tests in Upload.t are not testing the right thing
Summary: Some tests in Upload.t are not testing the right thing
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Chris Cormack
QA Contact: Testopia
URL:
Keywords:
Depends on: 17501
Blocks: 22508
  Show dependency treegraph
 
Reported: 2019-04-05 00:59 UTC by Jonathan Druart
Modified: 2019-05-22 13:21 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22644: Highlight the problem (1.46 KB, patch)
2019-04-05 01:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-04-05 00:59:17 UTC
Wondering if there is not something wrong or missing in t/db_dependent/Upload.t, the keep_file flag is not testing if the file is really kept.
Also we are not testing if the files have been correctly created.
Comment 1 Jonathan Druart 2019-04-05 01:00:54 UTC
Created attachment 87429 [details] [review]
Bug 22644: Highlight the problem
Comment 2 Marcel de Rooy 2019-05-13 07:20:31 UTC
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.
Comment 3 Jonathan Druart 2019-05-13 12:52:41 UTC
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.
Comment 4 Jonathan Druart 2019-05-22 04:46:43 UTC
Marcel, what do you think? How could I provide tests for bug 22508 without having this being fixed? Any ideas?
Comment 5 Marcel de Rooy 2019-05-22 07:03:16 UTC
(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.
Comment 6 Marcel de Rooy 2019-05-22 13:21:48 UTC
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?