diff --git a/src/osw/wtsite.py b/src/osw/wtsite.py index 8193c3ed..5fa8deca 100644 --- a/src/osw/wtsite.py +++ b/src/osw/wtsite.py @@ -933,6 +933,13 @@ def create_page_package(self, param: CreatePagePackageParam): WtSite.GetPageParam(titles=titles_to_fetch, parallel=param.parallel) ).pages ) + + # get_page appends in completion order when fetching in parallel, so the + # page that claims a file referenced by several pages would vary between + # runs. Restore the configured order. + title_rank = {title: rank for rank, title in enumerate(added_titles)} + pages.sort(key=lambda p: (title_rank.get(p.title, len(title_rank)), p.title)) + added_file_titles = [] page_dumps = {} page_files = {} @@ -957,8 +964,10 @@ def create_page_package(self, param: CreatePagePackageParam): if debug and len(ignored_files_pages) > 0: print(f"Ignored: {ignored_files_pages}") referenced_file_pages = included_file_pages - # find those files that are not already in the package - page_files[page.title] = list( + # find those files that are not already in the package. sorted() + # because the set difference and find_file_page_refs_in_slots + # both return an order that varies between runs + page_files[page.title] = sorted( set(referenced_file_pages) - set(added_file_titles) ) added_file_titles = list(set(added_file_titles + referenced_file_pages)) diff --git a/tests/test_wtsite_create_page_package.py b/tests/test_wtsite_create_page_package.py index d2d3c028..da693f92 100644 --- a/tests/test_wtsite_create_page_package.py +++ b/tests/test_wtsite_create_page_package.py @@ -1,10 +1,16 @@ -"""Unit tests for the PagePackageConfig.clear_content_dir purge flag. +"""Unit tests for the PagePackageConfig.clear_content_dir purge flag, and for +the deterministic ordering of packages.json (#172). Regression guard for #42: WtSite.create_page_package used to unconditionally shutil.rmtree() the content directory. clear_content_dir defaults to True (no behaviour change), but setting it to False must keep any existing content. + +Regression guard for #172: WtSite.create_page_package wrote the "pages" array +of packages.json in an order that varied between runs whenever +config.include_files was True, causing spurious diffs. """ +import json import threading import pytest @@ -27,6 +33,51 @@ def _make_fake_wtsite(): return ws +def _make_file_page(ws, title): + """A File: page whose dump() does not attempt a real file download.""" + page = WtPage(wtSite=ws, title=title, do_init=False) + page.exists = True + page.dump = lambda config, _title=title: package.PagePackagePage( + name=_title.split(":")[-1], namespace="NS_FILE", slots={} + ) + return page + + +def _find_order_sensitive_file_titles(): + """Two File: titles for which list(set([a, b])) != list(set([b, a])) under + the current process's (randomized) string hash seed. Used to reproduce the + hash-order dependent bug without relying on a specific PYTHONHASHSEED.""" + for i in range(1000): + for j in range(i + 1, 1000): + a, b = f"File:F{i}.png", f"File:F{j}.png" + if list({a, b}) != list({b, a}): + return a, b + raise RuntimeError("could not find an order-sensitive pair of file titles") + + +def _make_bundle_and_config(tmp_path, subdir, titles, include_files=True): + target = tmp_path / subdir + target.mkdir(parents=True, exist_ok=True) + bundle = package.PagePackageBundle( + packages={ + "TestPkg": package.PagePackage( + globalID="org.test.TestPkg", + description="test package", + version="0.0.1", + baseURL="https://example.org/", + ) + } + ) + return package.PagePackageConfig( + name="TestPkg", + config_path=target / "packages.json", + content_path=target / "content", + bundle=bundle, + titles=titles, + include_files=include_files, + ) + + def _make_config(tmp_path, clear_content_dir): bundle = package.PagePackageBundle( packages={ @@ -84,3 +135,99 @@ def test_create_page_package_honours_clear_content_dir(tmp_path, clear_content_d assert not marker.exists() else: assert marker.exists() + + +def test_create_page_package_per_page_file_order_is_deterministic(tmp_path): + """Regression test for #172: the order in which a page's file references + are discovered must not change where they end up in packages.json.""" + file_a, file_b = _find_order_sensitive_file_titles() + extra_file = "File:Extra.png" + + def _run(subdir, reversed_refs): + ws = _make_fake_wtsite() + item1 = WtPage(wtSite=ws, title="Item:OSW1", do_init=False) + item2 = WtPage(wtSite=ws, title="Item:OSW2", do_init=False) + order = [file_b, file_a] if reversed_refs else [file_a, file_b] + item1.find_file_page_refs_in_slots = lambda slots=None: list(order) + item2.find_file_page_refs_in_slots = lambda slots=None: [extra_file] + + offline_pages = { + "Item:OSW1": item1, + "Item:OSW2": item2, + file_a: _make_file_page(ws, file_a), + file_b: _make_file_page(ws, file_b), + extra_file: _make_file_page(ws, extra_file), + } + + config = _make_bundle_and_config( + tmp_path, subdir, titles=["Item:OSW1", "Item:OSW2"] + ) + ws.create_page_package( + WtSite.CreatePagePackageParam( + config=config, + offline_pages=offline_pages, + debug=False, + ) + ) + return json.loads(config.config_path.read_text(encoding="utf-8")) + + data_1 = _run("run1", reversed_refs=False) + data_2 = _run("run2", reversed_refs=True) + + names_1 = [p["name"] for p in data_1["packages"]["TestPkg"]["pages"]] + names_2 = [p["name"] for p in data_2["packages"]["TestPkg"]["pages"]] + + assert names_1 == names_2 + + # Item:OSW1's own files sit between its dump and Item:OSW2's dump, and are + # in sorted order. + expected_file_names = sorted([file_a.split(":")[-1], file_b.split(":")[-1]]) + assert names_1[1:3] == expected_file_names + + +def test_create_page_package_shared_file_always_attaches_to_first_page(tmp_path): + """Regression test for #172: a file referenced by several pages must + always be attached to the same (first-configured) page, regardless of the + order in which get_page's parallel fetch happens to complete.""" + shared_file = "File:Shared.png" + + def _run(subdir, reverse_fetch_order): + ws = _make_fake_wtsite() + item1 = WtPage(wtSite=ws, title="Item:OSW1", do_init=False) + item2 = WtPage(wtSite=ws, title="Item:OSW2", do_init=False) + item1.find_file_page_refs_in_slots = lambda slots=None: [shared_file] + item2.find_file_page_refs_in_slots = lambda slots=None: [shared_file] + + pages_by_title = { + "Item:OSW1": item1, + "Item:OSW2": item2, + shared_file: _make_file_page(ws, shared_file), + } + + def fake_get_page(param): + titles = list(param.titles) + if reverse_fetch_order: + titles = list(reversed(titles)) + selected = [pages_by_title[t] for t in titles] + return WtSite.GetPageResult(pages=selected, errors=[]) + + ws.get_page = fake_get_page + + config = _make_bundle_and_config( + tmp_path, subdir, titles=["Item:OSW1", "Item:OSW2"] + ) + ws.create_page_package( + WtSite.CreatePagePackageParam(config=config, debug=False) + ) + return json.loads(config.config_path.read_text(encoding="utf-8")) + + data_normal = _run("normal", reverse_fetch_order=False) + data_reversed = _run("reversed", reverse_fetch_order=True) + + names_normal = [p["name"] for p in data_normal["packages"]["TestPkg"]["pages"]] + names_reversed = [p["name"] for p in data_reversed["packages"]["TestPkg"]["pages"]] + + assert names_normal == names_reversed + # Shared.png must be attached to Item:OSW1 (first in config.titles), even + # when Item:OSW2 is fetched first. + assert names_normal == ["OSW1", "Shared.png", "OSW2"]