autoremove files created during testing

[antgonza]

No description provided.

[coveralls]

coverage: 92.155% (+0.002%) from 92.153%
when pulling e2a6c51 on antgonza:autoremove-files-created-during-testing
into e0e7480 on qiita-spots:dev.

[sjanssen2]

Hi @antgonza,

I very much like the idea that also files get cleaned up, when an API reset is triggered in tests!
But we might want to think a bit bigger here:

1. there are some tests, that first create necessary artifact files, execute the test and then delete these files "locally", i.e. within the plugin test code, e.g. https://github.com/qiita-spots/qp-target-gene/blob/858436ee6d4a1fccd35faee0b156a12953b5774a/qp_target_gene/split_libraries/tests/test_split_libraries_fastq.py#L353-L363. Thus, NOT checking the status of artifacts with ID < 9 might not really reset all files as expected and it is maybe not the pure presence/absence but the content of a file that needs to be reset?
2. at the moment, the developer of a plugin needs to adhere to the schema that he registers tmp files in an array and for tear down removes all these files (and or directories). This results in duplication of code and need to remember / educate that this schema exists. Maybe it would be easier to erase / replace all tests files of qiita main (this is the server code NOT the plugin) once reset via API is triggered?
3. when separating plugins from qiita main, the plugin itself has no longer direct access to the BASE_DATA_DIR. This
   1. requires additional, explicit file transfer in the test
   2. renders deletion of these files impossible for the plugin IF we do not expose according functions in qiita main, which I am still hesitant to do
4. at the moment, I only see the need to delete files in qiita main from within a plugin (the real plugin, not test code), namely here: https://github.com/jlab/qtp-sequencing/blob/5d5358b26f9a3d205bc6acd54f4cf34fb3bf16e6/qtp_sequencing/validate.py#L76-L94 This is not even an artifact file, but refers to some sequence file in the upload directory; thus, we might want to also put emphasis on this directory when cleaning up tests?

[antgonza]

Thank you for the summary, really impressive that you tracked all that.

1. Poor decision in retrospect; anyway, I think this is not important and it should not alter anything here. Those created files will not be available for other plugins as they are not listed in the database (qiita actually doesn't know about them). BTW note that they are being inserted in an existing artifact. However, if you feel strongly, we could call the purge_filepaths and/or quick_mounts_purge to clean as much as possible.
2. I don't follow this point, are you saying that we should remove all the code that does clean up in plugins or something else?
3. If I follow, this is intentional - plugins should not have access directly to the files and should rely on Qiita to let them know where they live. For example, in our testing environment we have access to the real data via a only read mount but also provide another mount (read/write) to write the new files being created. This is a feature.
4. This is only for testing and the plugins do not have the possibility of removing anything from Qiita artifacts. The code you point only works on the working or upload directory, never on the artifact folders.

What do you think?

[sjanssen2]

Hi @antgonza,

1. I didn't know about purge_filepaths or quick_mounts_purge. I think this would be very handy to be called once a API reset is triggered. How much overhead would that incur for all the tests in terms of walltime. I wonder if github runners can still execute in given time. One thing that might be missed, is if an existing file has been changed by a plugin test - if I am not mistaken.
2. Every plugin test has code like in the following. Wouldn't it be nice to move this once into API reset (in qiita) or the PluginTestCase definition in https://github.com/jlab/qiita_client/blob/16671cabee644a1ddc2ebca7785e9a0fed785525/qiita_client/testing.py#L23-L46?
   1. deblur https://github.com/jlab/qp-deblur/blob/80467b7c76b6177abb577e19c83266ee54dbcd79/qp_deblur/tests/test_deblur.py#L44 and https://github.com/jlab/qp-deblur/blob/80467b7c76b6177abb577e19c83266ee54dbcd79/qp_deblur/tests/test_deblur.py#L63-L68
   2. target-gene https://github.com/jlab/qp-target-gene/blob/858436ee6d4a1fccd35faee0b156a12953b5774a/qp_target_gene/tests/test_trimming.py#L27-L35 and https://github.com/jlab/qp-target-gene/blob/858436ee6d4a1fccd35faee0b156a12953b5774a/qp_target_gene/tests/test_pick_otus.py#L33-L47
   3. https://github.com/jlab/qtp-biom/blob/2879fedcdac7123ba8319853851479849bc3e01e/qtp_biom/tests/test_plugin.py#L27-L36
3. I fully agree on "plugins should not have access directly to the files" in productive mode. However, in test mode, plugins already have the ability to wipe out the DB through the API reset. Only for cleaning up tests, I suggest that we either need an explicit mechanism for the plugin to delete intermediate test files OR implicitly accomplish that by extending the API reset to the files in BASE_DATA_DIR. And I think this is the intention of your PR. Do you think your PR also covers files / directories created by qtp-job-output-folder (here, individual files are NOT in the qiita DB).
4. You are right, this affects the upload directory. Conceptually, I would see this upload directory under the control of qiita main (not individual plugins), similar to the BASE_DATA_DIR. If so, in the uncoupling situation, the plugin cannot remove files in this directory. Which might be fine, but will change the current behavior of qiita OR fails the current tests that are in place for qtp-sequencing :-/ If a user uploads a fastq file, the plugin will a) create a compressed version fastq.gz and b) delete the fastq source file. In my opinion, we should go with the changed behavior (as uncompressed fastq files are unusual anyway) and avoid implementing a potentially dangerous deletion mechanism.

[sjanssen2]

I have tested the effect on uncoupled plugins. In fact, it resolves the only outstanding issue for qtp-job-output-folder. Amazing! Thus, remote deletion in qiita is really not necessary and I'd be happy if your PR can be merged. Thanks a lot @antgonza

[antgonza]

Awesome! Thank you for testing!