test: fix delete file test

[HKuz]

Addresses #58

Still a work in progress, but pushing up the refactored code as a start.

[agritheory]

OK, I likewise ran into a bunch of issues as I was working on this.

• Frappe's rename_doc API has an name error where you can set validation to False and then old_doc becomes undefined. Poor testing on their part, strategy for resolving this is unclear.
• The File object is not setting the file_url and s3_key fields inside the save hooks, so we don't get them for the test. file.reload doesn't seem to bring them up either. Something else may be going on here.
• In the cases where we're uploading the same file to test for association with the original, the returned file PK is no longer valid since its been merged with the first/oldest file with a matching PK. This is likely a problem in the assumptions of the helper function and we need to fetch the File record with retrieve instead.

[agritheory]

@HKuz @Alchez @MyuddinKhatri Order of operations issue with the test and the File Association feature, plus a couple of things that make me say "that's not good".

1. s3_key and file_url do not seems to begetting set soon enough.

_____________________________________________________________________________________ test_upload_file _____________________________________________________________________________________
example_file_record_0 = PosixPath('/home/tyler/uhdei/apps/cloud_storage/cloud_storage/tests/fixtures/aticonrusthex.png')

    @mock_s3
    def test_upload_file(example_file_record_0):
        frappe.set_user("Administrator")
        file = create_upload_file(example_file_record_0)  # File has been saved and the record of it exists in DB at this point
        assert frappe.db.exists("File", file.name)
        assert file.attached_to_doctype == "User"
        assert file.attached_to_name == "Administrator"
        assert file.attached_to_field == None
        assert file.folder == "Home"
        assert file.file_name == "aticonrusthex.png"
        assert file.content_hash is None
        assert (
                file.file_url == "/api/method/retrieve?key=test_folder/User/Administrator/aticonrusthex.png"
        )
        assert file.is_private == 0  # makes the delete file test easier
        # FIXME Failing - related to db commit?
>       assert file.s3_key is not None # if it exists in the DB, why does the S3 key not exist? file_url should also be set
E    assert None is not None
E     +  where None = <CustomFile: d59ce0447b>.s3_key

../apps/cloud_storage/cloud_storage/tests/test_file.py:71: AssertionError

2. I'm not sure how we correctly mock the File record returning a new value at this point. The stratgey for uploading a duplicate file is to add the association and force rename, so there's only one parent.

________________________________________________________________________ test_upload_file_with_multiple_association ________________________________________________________________________

example_file_record_0 = PosixPath('/home/tyler/uhdei/apps/cloud_storage/cloud_storage/tests/fixtures/aticonrusthex.png')

    @mock_s3
    def test_upload_file_with_multiple_association(example_file_record_0):
>       _file = create_upload_file(example_file_record_0)

../apps/cloud_storage/cloud_storage/tests/test_file.py:87:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../apps/cloud_storage/cloud_storage/tests/test_file.py:40: in create_upload_file
    file = frappe.call("frappe.handler.upload_file")
../apps/frappe/frappe/__init__.py:1611: in call
    return fn(*args, **newargs)
../apps/frappe/frappe/handler.py:234: in upload_file
    return frappe.get_doc(
../apps/frappe/frappe/model/document.py:307: in save
    return self._save(*args, **kwargs)
../apps/frappe/frappe/model/document.py:329: in _save
    return self.insert()
../apps/frappe/frappe/model/document.py:279: in insert
    self.run_method("after_insert")
../apps/frappe/frappe/model/document.py:928: in run_method
    out = Document.hook(fn)(self, *args, **kwargs)
../apps/frappe/frappe/model/document.py:1280: in composer
    return composed(self, method, *args, **kwargs)
../apps/frappe/frappe/model/document.py:1262: in runner
    add_to_return_value(self, fn(self, *args, **kwargs))
../apps/frappe/frappe/model/document.py:925: in fn
    return method_object(*args, **kwargs)
../apps/cloud_storage/cloud_storage/cloud_storage/overrides/file.py:124: in after_insert
    rename_doc(
../apps/frappe/frappe/model/rename_doc.py:154: in rename_doc
    new = validate_rename(
../apps/frappe/frappe/model/rename_doc.py:353: in validate_rename
    frappe.throw(_("No changes made because old and new name are the same.").format(old, new))
../apps/frappe/frappe/__init__.py:541: in throw
    msgprint(
../apps/frappe/frappe/__init__.py:509: in msgprint
    _raise_exception()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    def _raise_exception():
        if raise_exception:
                if inspect.isclass(raise_exception) and issubclass(raise_exception, Exception):
>                       raise raise_exception(msg)
E      frappe.exceptions.ValidationError: No changes made because old and new name are the same.

../apps/frappe/frappe/__init__.py:455: ValidationError

[Alchez]

@agritheory @HKuz @MyuddinKhatri I've added a commit that should fix all the failing tests locally with a bench running, but it's failing trying to manage the S3 client on our CI. I'm not sure exactly how to set that up.

For some more context, the pytest suite wasn't properly simulating the client request, with the file object being different (BytesIO) from what Frappe was expecting (Werkzeug's FileStorage-like). Because of that, our Cloud Storage hooks and overrides weren't triggering at all (causing the S3 Key to not be set, causing both the upload and delete tests to fail).

[agritheory]

There we go. Thanks for chasing this down

[agritheory]

OK, after a lot of frustration and unintentionally re-doing Heather's work, I've gotten the tests running, but there's a new error I haven't seen before.

It looks like we need to also run a Redis instance in the CI, like this.

[Alchez]

@agritheory Seems like the bench setup was just missing a Redis setup config. The tests are running and passing now.