Make the macOS CI leg say something - #2
Conversation
…en saying The macOS leg has been red since it was added, so a new break on that platform has looked exactly like the old ones - which is the same as having no leg. Four failures, and only one of them was really about macOS. `test_full_mode_still_tracks_it` drives the full-mode target lookup, which resolves through `classify` into `inject.py`, which binds user32 at import. That is one of the six Win32 mechanisms the workflow comment already names; it was simply missing the guard the others carry. The two `TestAHotkeysBlockIsInertWhereNothingIsRegistered` cases were the interesting ones, and not a platform problem at all. `launch()` patches `sys.platform` to "win32" - and `sys.platform` is not Flow's variable. On a real macOS runner the *stdlib* reads it too: `shutil.which` takes its Windows branch and calls into `_winapi`, which is None, and the traceback ends on a line no part of Flow wrote. `available()` is patched out instead, which is where that call came from and which has nothing to do with what the class asserts. It is faster too - resolving CLIs probes subprocesses. The font advance is a property of a font only installed on Windows. `_load_fonts` registers the bundled Plex weights through GDI and returns early everywhere else, so Tk substitutes and the test measured the substitute - 10 against 7. Skipped on the substitution rather than on the platform, because the question is whether the face resolved, and a Windows machine missing the file deserves the same answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 164dcf8d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # raised from a line no part of Flow wrote. Which agent CLI happens to be | ||
| # installed has nothing to do with what this class asserts. | ||
| with mock.patch.object(sys, "platform", platform), \ | ||
| mock.patch.object(mod, "available", return_value=[]), \ |
There was a problem hiding this comment.
On macOS, the two tests that use this helper's default platform="win32" still fail: after available() returns, main() calls the separately imported unverified(), which traverses detected() → resolve() → shutil.which(). Because sys.platform is still spoofed to win32, that call reaches the unavailable _winapi.NeedCurrentDirectoryForExePath just as available() did. Patch unverified() as well (or patch the resolver beneath both calls), otherwise this change does not make the macOS CI leg green.
Useful? React with 👍 / 👎.
| if f.actual("family") != ui.FONT_TRACE[0]: | ||
| self.skipTest(f"{ui.FONT_TRACE[0]} not installed; " | ||
| f"Tk substituted {f.actual('family')!r}") |
There was a problem hiding this comment.
When _load_fonts() regresses on Windows—for example, a font filename or GDI registration call changes—Tk substitutes another family and this new branch marks the test skipped, so the only test covering both bundled-font resolution and the hardcoded LABEL_ADV silently disappears. Since the font files are repository assets and the product relies on their metrics, substitution should only be skipped on unsupported platforms; on Windows it should fail so a broken font-loading path cannot ship unnoticed.
Useful? React with 👍 / 👎.
The first attempt patched `available()` and CI found the other road: `main` reaches the same `shutil.which` through `unverified()` as well, so half the failure survived. `resolve` is the single choke point - the only `shutil.which` in the module, and what every lookup goes through - so it is stubbed there and neither caller matters. Proved without the runner this time. With `shutil.which` rigged to raise the exact AttributeError a Mac raises when `sys.platform` has been patched to "win32", all five cases pass, which is the same thing the macOS leg is about to say more slowly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
The macOS leg has been red since it was added, so a new break on that platform looks exactly like the old ones — which is the same as having no leg. This is four fixes and no behaviour change;
flow/is untouched.Only one of the four was really about macOS:
test_lite.test_full_mode_still_tracks_itclassifyintoinject.pyand binds user32 at import. One of the six Win32 mechanisms the workflow comment already names — it was just missing the guard the others carry.test_main×2launch()patchessys.platformto"win32", andsys.platformis not Flow's variable — on a real Mac the stdlib reads it too.shutil.whichtakes its Windows branch into_winapi, which isNone, and the traceback ends on a line no part of Flow wrote.test_pillfont advance_load_fontsreturns early elsewhere, so Tk substitutes and the test measured the substitute — 10 against 7.The
test_mainpair is patched atavailable()rather than by skipping: that call is whereshutil.whichcame from, it has nothing to do with what the class asserts, and resolving CLIs probes subprocesses so the tests get faster too.The font check now skips on the substitution rather than on the platform — the question is whether the face resolved, and a Windows machine missing the file deserves the same answer.
Wanted before #1, which adds macOS window fixes and a native macOS decoder and needs that leg to mean something.