Skip to content

fix(test): make the _url_copy tests type-clean and machine-independent - #773

Merged
martinsifra merged 1 commit into
mainfrom
fix/ty-url-copy-tests
Sep 17, 2026
Merged

martinsifra merged 1 commit into
mainfrom
fix/ty-url-copy-tests

Conversation

@martinsifra

@martinsifra martinsifra commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

main is currently red: the CI check job's "Type check (ty)" step and the Windows job's "Full test suite on Windows" both fail at f5eb4c0, so every open PR inherits a failing build. Test-only fix, no production behaviour changes.

Both failures come from tests/test_url_copy.py (shipped by #772, which was merged with its own CI already red).

1. Job check — "Type check (ty)"

The tests read captured output as console.file.getvalue(). Rich declares Console.file as IO[str], which has no getvalue — three unresolved-attribute errors. Added an _output() helper that narrows it with cast(io.StringIO, console.file), which is the idiom tests/test_output.py already uses at 23 read sites; wrapped in a helper only because this file reads the buffer from several tests.

2. Windows job — two tests that could only pass in a bare Linux container

copier=None means "detect a backend", not "there is none" (_url_copy.py:123: self._copier = copier if copier is not None else detect_clipboard()). So test_disabled_without_backend and test_on_prompt_silent_when_disabled asserted "disabled" — which holds only where detect_clipboard() finds nothing.

Windows has clip, macOS always has pbcopy, WSL has clip.exe, a Linux desktop has xclip/xsel/wl-copy. So these tests were green in exactly one of the three CI jobs and red for any developer on a real workstation — that is both why the Windows job failed and how this surfaced locally. Both now stage the no-backend case by monkeypatching detect_clipboard, so the assertion is about the code rather than about the machine.

Verification

  • make typecheck — down to the one pre-existing scripts/hatch_build.py warning
  • make check — exit 0, 6798 passed, 12 skipped
  • tests/test_url_copy.py — 10 passed, on a machine that does have a clipboard backend (WSL clip.exe), i.e. one that reproduced the original failure

Deliberately out of scope

copier=None being overloaded — "not given" and, as these tests assumed, "none exists" — is arguably the root cause, but it is production API and the documented behaviour is correct; only the tests' assumption was wrong. Left for a follow-up so this stays a minimal, test-only unblock.

Found while preparing the 0.95.0 release PR (#774), which cannot go out on a red main.

🤖 Generated with Claude Code

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: auto_approve (risk 1/5) · profile _default

Test-only change to a single test file, no source tree touched — auto-approve.

@martinsifra martinsifra mentioned this pull request Sep 16, 2026
9 of 10 tasks
`main` is red: CI's `uv run ty check` step fails on three
`console.file.getvalue()` reads in tests/test_url_copy.py. Rich declares
`Console.file` as `IO[str]`, which has no `getvalue`. Added an `_output()`
helper that narrows it back with `cast(io.StringIO, ...)` -- the idiom
tests/test_output.py already uses at its 23 read sites -- and routed the
three reads through it.

Two tests were also environment-dependent and failed for essentially
every developer while passing in CI. `copier=None` means "detect a
backend" (_url_copy.py:123), not "there is none", so
`test_disabled_without_backend` and `test_on_prompt_silent_when_disabled`
asserted "disabled" on a machine where `detect_clipboard()` finds nothing
-- true only in a bare container. On macOS (`pbcopy` always present),
Windows (`clip`), WSL (`clip.exe`) or any Linux desktop with
`xclip`/`wl-copy` they failed, which is how this was noticed and which is
why the Windows CI job went red too. Both now stage the no-backend case
by monkeypatching `detect_clipboard`.

Test-only; no production behaviour changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@keboola-pr-reviewer-bot
keboola-pr-reviewer-bot dismissed their stale review September 17, 2026 00:18

Dismissing prior approval — a new commit was pushed and this review was for an earlier SHA. Run @keboola-pr-reviewer-bot review to get a fresh verdict.

@keboola-pr-reviewer-bot

Copy link
Copy Markdown

New commit on cc23cd4 — dismissed 1 stale bot approval. Comment @keboola-pr-reviewer-bot review when you want a fresh review.

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: auto_approve (risk 1/5) · profile _default

Test-only fix to one test file makes assertions type-clean and machine-independent; no source or runtime behavior changes.

@martinsifra
martinsifra merged commit 56120a2 into main Sep 17, 2026
5 checks passed
@martinsifra
martinsifra deleted the fix/ty-url-copy-tests branch September 17, 2026 00:30
martinsifra added a commit that referenced this pull request Sep 17, 2026
Covers everything merged since v0.94.0: #768 (uv audit / uv >= 0.12.15),
#772 (device-login "press c to copy") and #773 (test housekeeping).

No `vNEXT` placeholders were outstanding (`make vnext-check` clean), and
no `whatsnew.ts` entry is added -- nothing under `web/` changed this
release, so the popup correctly has nothing new to show.

gotchas.md: #772 reordered the device-login panel without updating the
doc surface. The 0.92.0 "one-click link is conditional" gotcha quoted the
old label ("Or open this link (code pre-filled):") and the old ordering,
so a relay keying on either now picks the wrong line -- rewritten, plus a
new entry for `press c to copy` being absent in exactly the situations an
agent drives (`--json`, no TTY, no clipboard command).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@soustruh soustruh mentioned this pull request Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants