Fix: schedule_for_cleanup was not called correctly in test_wiki #1369
Fix: schedule_for_cleanup was not called correctly in test_wiki #1369linglp wants to merge 16 commits into
Conversation
| download_location=download_dir, | ||
| synapse_client=self.syn, | ||
| ) | ||
| schedule_for_cleanup(downloaded_path) |
There was a problem hiding this comment.
We don't need this because temporary directory is already being cleaned up above.
There was a problem hiding this comment.
Pull request overview
This PR updates the async Wiki integration tests to call schedule_for_cleanup with values that the integration-test cleanup harness can actually delete, eliminating noisy “Don’t know how to clean …” messages (notably from passing numeric wiki IDs and other non-cleanable values).
Changes:
- Replaced wiki cleanup calls from raw numeric wiki ID strings to
WikiPageobjects (so_cleanupcan calldelete_async). - Removed cleanup scheduling for non-cleanable return values (pre-signed URL strings, handle dicts, header/history lists, order-hint objects).
- Standardized tests to use
self.schedule_for_cleanup(via theinitautouse fixture) instead of threadingschedule_for_cleanupthrough many test signatures.
Comments suppressed due to low confidence (3)
tests/integration/synapseclient/models/async/test_wiki_async.py:309
- This test creates a >8MiB temp file via
make_bogus_uuid_file()and writes to it, but the file path is no longer scheduled for cleanup. That can leave large files on disk after local runs; schedulefilenamefor cleanup once the upload is complete.
# Create a temporary file for attachment with > 8 MiB
filename = utils.make_bogus_uuid_file()
with open(filename, "wb") as f:
f.write(b"\0" * (9 * 1024 * 1024))
tests/integration/synapseclient/models/async/test_wiki_async.py:349
- This fixture renames the temp file to
gz_filenameand writes it, but the local gz file is not scheduled for cleanup anymore. Sincemake_bogus_uuid_file()usesdelete=False, consider schedulinggz_filename(and/or removingfilenameif it still exists) for cleanup to avoid leaking temp files.
# Create a gzipped file
filename = utils.make_bogus_uuid_file()
# Rename to add .gz extension
gz_filename = filename + ".gz"
os.rename(filename, gz_filename)
with gzip.open(gz_filename, "wt") as f:
f.write("hello world\n")
tests/integration/synapseclient/models/async/test_wiki_async.py:583
- This fixture creates and writes
md_gz_filenameviamake_bogus_uuid_file()/delete=False, but no longer schedules that local file for cleanup. Either schedulemd_gz_filenamefor cleanup or remove this unused temp file creation to prevent leaking files during integration test runs.
filename = utils.make_bogus_uuid_file()
# Rename to add .md.gz extension
md_gz_filename = filename.replace(".txt", ".md.gz")
os.rename(filename, md_gz_filename)
with gzip.open(md_gz_filename, "wt") as f:
f.write("# Test Wiki\n\nThis is test content.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| retrieved_wiki = await WikiPage( | ||
| owner_id=root_wiki.owner_id, id=root_wiki.id | ||
| ).get_async(synapse_client=self.syn) | ||
| schedule_for_cleanup(retrieved_wiki.id) |
There was a problem hiding this comment.
Here, even for deleting wiki, the code should be:
schedule_for_cleanup(retrieved_wiki)
But the wiki shouldn't be deleted because wiki_page_fixture is a class level fixture. If we delete it here, then this would potentially affect other tests
|
I had one question, but overall LGTM! |
| schedule_for_cleanup(retrieved_order_hint) | ||
| assert retrieved_order_hint.id_list == header_ids | ||
| assert len(retrieved_order_hint.id_list) >= 1 | ||
|
|
There was a problem hiding this comment.
No need to clean up wiki pages here because the project will be cleaned up anyway:
@pytest.fixture(scope="class")
async def wiki_page_fixture(
self, syn: Synapse, schedule_for_cleanup: Callable[..., None]
) -> WikiPage:
"""Create a root wiki page fixture shared across tests in this class."""
project = Project(name=f"Test Wiki Project_" + str(uuid.uuid4()))
project = await project.store_async(synapse_client=syn)
schedule_for_cleanup(project.id)
wiki_title = f"Root Wiki Page {str(uuid.uuid4())}"
wiki_markdown = "# Root Wiki Page\n\nThis is a root wiki page."
wiki_page = WikiPage(
owner_id=project.id,
title=wiki_title,
markdown=wiki_markdown,
)
root_wiki = await wiki_page.store_async(synapse_client=syn)
return root_wiki
| ) | ||
| wiki_page = await wiki_page.store_async(synapse_client=self.syn) | ||
| schedule_for_cleanup(wiki_page.id) | ||
| self.schedule_for_cleanup(wiki_page) |
There was a problem hiding this comment.
Since wiki is not an entity, would this also fail to cleanup?
There was a problem hiding this comment.
Hi @BryanFauble ! I later realized that wiki page was handled explicitly by the code here. I will remove WikiOrderHint, WikiHistorySnapshot, and WikiHeader from conftest because they don't have delete function and can't be deleted that way
| download_location=download_dir, | ||
| synapse_client=self.syn, | ||
| ) | ||
| schedule_for_cleanup(downloaded_path) |
There was a problem hiding this comment.
Could probably be kept to cleanup the file.
| ) | ||
| sub_wiki = await wiki_page.store_async(synapse_client=syn) | ||
| schedule_for_cleanup(sub_wiki.id) | ||
| schedule_for_cleanup(sub_wiki) |
There was a problem hiding this comment.
| ) | ||
| sub_wiki = await wiki_page.store_async(synapse_client=syn) | ||
| schedule_for_cleanup(sub_wiki.id) | ||
| schedule_for_cleanup(sub_wiki) |
There was a problem hiding this comment.
| ) | ||
| sub_wiki = await wiki_page.store_async(synapse_client=syn) | ||
| schedule_for_cleanup(sub_wiki.id) | ||
| schedule_for_cleanup(sub_wiki) |
There was a problem hiding this comment.
| owner_id=root_wiki.owner_id, id=updated_wiki.id, title="Version 2" | ||
| ).store_async(synapse_client=syn) | ||
| schedule_for_cleanup(updated_wiki.id) | ||
| schedule_for_cleanup(updated_wiki) |
There was a problem hiding this comment.
BryanFauble
left a comment
BryanFauble
left a comment
There was a problem hiding this comment.
A few things to patch up, otherwise LGTM!
Problem:
I saw this in the Github action run log:
Reason:
schedule_for_cleanupwas not called correctly in test_wikiBelow can't be cleaned up:
Solution:
Updated wiki async integration tests to only schedule cleanup for actual deletable entities or local filesystem paths. Specifically:
self.schedule_for_cleanupinstead of passingschedule_for_cleanupas a parameter`