[PR #3116] [MERGED] test: fix the install-flags structural guard failing on main #3030

New issue

Closed

opened 2026-10-28 21:14:39 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-28 21:14:39 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/ComfyUI-Manager/pull/3116
Author: @Dhevenddra
Created: 7/31/2026
Status: ✅ Merged
Merged: 8/2/2026
Merged by: @ltdrdata

Base: main ← Head: fix-structural-guard-on-main

📝 Commits (1)

c1bc425 test: fix the install-flags structural guard failing on main

📊 Changes

1 file changed (+11 additions, -6 deletions)

View changed files

📝 tests/test_install_flags_structural.py (+11 -6)

📄 Description

Summary

tests/test_install_flags_structural.py::JsCopyStructuralTest::test_generic_fallback_and_frozen_callers_unchanged fails on a clean checkout of main. Two separate defects, the second hidden behind the first. CI runs ruff check . only, so neither is visible there.

1. The guard counts file-wide but documents a narrower contract

Its own docstring says the copy has "exactly its two occurrences in handle403Response", but the assertion counts the whole file:

self.assertEqual(self.common_src.count(GENERIC_403_COPY), 2)

js/common.js now contains the sentence three times: twice inside handle403Response (lines 109 and 112, the real fallback) and once at line 722, where the batch-uninstall handler builds errorMsg for a 403 status. That third use is not a handle403Response caller, so it is outside what this guard covers, but the file-wide count still trips on it.

Scoped the count to the handle403Response block using the module's existing _js_function_block helper, which matches the documented intent. I checked the guard still bites: temporarily changing one of the two in-function occurrences makes the assertion fail as intended.

This one is not platform-specific. The count is 3 on Linux and Windows alike.

2. `read_text()` without an encoding fails on Windows

Once the count assertion passes, the test reaches:

for js_file in sorted((REPO_ROOT / "js").glob("*.js")):
    source = js_file.read_text()

Path.read_text() uses the locale encoding. On a cp1252 Windows install that raises UnicodeDecodeError on four files that contain emoji: comfyui-manager.js, comfyui-share-common.js, comfyui-share-copus.js, comfyui-share-openart.js.

Measured on both platforms from the same checkout:

platform: win32          preferred encoding: cp1252
count file-wide: 3       js files that fail bare read_text(): ['comfyui-manager.js', 'comfyui-share-common.js', 'comfyui-share-copus.js', 'comfyui-share-openart.js']

platform: linux          preferred encoding: UTF-8
count file-wide: 3       js files that fail bare read_text(): (none)

Passed encoding="utf-8" explicitly. I did the same for the other four read_text() calls in this file for consistency: they read manager_server.py, cm-cli.py, manager_migration.py and common.js, which are pure ASCII today, so they pass by luck rather than by contract, and would break the same way the first time someone adds a non-ASCII character.

Scope

Only tests/test_install_flags_structural.py changes. No shipped code is touched, and no other file in the repo uses a bare read_text().

Result

Before, on Windows: 1 failed, 37 passed, 14 skipped.
After: 38 passed, 14 skipped, 22 subtests passed.

ruff check . is clean.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/ComfyUI-Manager/pull/3116 **Author:** [@Dhevenddra](https://github.com/Dhevenddra) **Created:** 7/31/2026 **Status:** ✅ Merged **Merged:** 8/2/2026 **Merged by:** [@ltdrdata](https://github.com/ltdrdata) **Base:** `main` ← **Head:** `fix-structural-guard-on-main` --- ### 📝 Commits (1) - [`c1bc425`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/c1bc42583dd0b277bf5622146c74ef4cd6ce204b) test: fix the install-flags structural guard failing on main ### 📊 Changes **1 file changed** (+11 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `tests/test_install_flags_structural.py` (+11 -6) </details> ### 📄 Description ## Summary `tests/test_install_flags_structural.py::JsCopyStructuralTest::test_generic_fallback_and_frozen_callers_unchanged` fails on a clean checkout of `main`. Two separate defects, the second hidden behind the first. CI runs `ruff check .` only, so neither is visible there. ## 1. The guard counts file-wide but documents a narrower contract Its own docstring says the copy has "exactly its two occurrences **in handle403Response**", but the assertion counts the whole file: ```python self.assertEqual(self.common_src.count(GENERIC_403_COPY), 2) ``` `js/common.js` now contains the sentence three times: twice inside `handle403Response` (lines 109 and 112, the real fallback) and once at line 722, where the batch-uninstall handler builds `errorMsg` for a 403 status. That third use is not a `handle403Response` caller, so it is outside what this guard covers, but the file-wide count still trips on it. Scoped the count to the `handle403Response` block using the module's existing `_js_function_block` helper, which matches the documented intent. I checked the guard still bites: temporarily changing one of the two in-function occurrences makes the assertion fail as intended. This one is not platform-specific. The count is 3 on Linux and Windows alike. ## 2. `read_text()` without an encoding fails on Windows Once the count assertion passes, the test reaches: ```python for js_file in sorted((REPO_ROOT / "js").glob("*.js")): source = js_file.read_text() ``` `Path.read_text()` uses the locale encoding. On a cp1252 Windows install that raises `UnicodeDecodeError` on four files that contain emoji: `comfyui-manager.js`, `comfyui-share-common.js`, `comfyui-share-copus.js`, `comfyui-share-openart.js`. Measured on both platforms from the same checkout: ``` platform: win32 preferred encoding: cp1252 count file-wide: 3 js files that fail bare read_text(): ['comfyui-manager.js', 'comfyui-share-common.js', 'comfyui-share-copus.js', 'comfyui-share-openart.js'] platform: linux preferred encoding: UTF-8 count file-wide: 3 js files that fail bare read_text(): (none) ``` Passed `encoding="utf-8"` explicitly. I did the same for the other four `read_text()` calls in this file for consistency: they read `manager_server.py`, `cm-cli.py`, `manager_migration.py` and `common.js`, which are pure ASCII today, so they pass by luck rather than by contract, and would break the same way the first time someone adds a non-ASCII character. ## Scope Only `tests/test_install_flags_structural.py` changes. No shipped code is touched, and no other file in the repo uses a bare `read_text()`. ## Result Before, on Windows: `1 failed, 37 passed, 14 skipped`. After: `38 passed, 14 skipped, 22 subtests passed`. `ruff check .` is clean. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-28 21:14:39 +00:00