Run Playwright tests in CI for shell integration - #45
Open
savetheclocktower wants to merge 3 commits into
Open
Conversation
Reads the package name from package.json at runtime (via the same __dirname-based PACKAGE_ROOT walk-up pattern already used in shell-integration/index.ts) rather than repeating the literal string 'terminal' across the codebase. Config keys, storage keys, and URIs that were built from that hardcoded string now derive from PACKAGE_NAME instead, so they can never drift out of sync with the package's actual identity.
Three real bugs surfaced while getting a real end-to-end shell-integration test running reliably: - TerminalElement's IntersectionObserver disconnected itself only after awaiting createTerminal(), not before. Intersection changes can be delivered as more than one callback invocation before the first one's own continuation resumes, so a second invocation could slip through and trigger a redundant, concurrent createTerminal() call before the first ever reached its disconnect(). Disconnecting synchronously, on the first qualifying entry, closes that window. - decoration.onRender() (used to anchor hover tooltips) isn't a one-shot "first paint" hook - XTerm re-invokes it on every repaint of that decoration (scroll, resize, cursor blink). The callback unconditionally re-added a tooltip via atom.tooltips.add() on every one of those repaints, stacking up duplicate tooltip instances for a single hover. - createTerminal() and restartPtyProcess() both had "wait for the current call, then start a new one anyway" reentrancy handling, which doesn't actually serialize concurrent callers: when several calls pile up awaiting the same in-flight promise, each one wakes up and unconditionally kicks off its own new run, racing the others. Coalescing onto the in-flight promise (return instead of just await) fixes this for both. Also switched their cleanup from .then() to .finally() - a rejected call was otherwise never clearing the field, wedging every subsequent call behind a dead promise forever. Along with the corresponding spec updates: two new regression tests for the view-provider path, the test helper reworked to rely on the (now properly guarded) IntersectionObserver instead of racing it with a redundant explicit createTerminal() call, and findShellIntegrationAddon() made robust to more than one addon having been loaded.
…ish/pwsh Real end-to-end coverage: a real installed Pulsar, this checkout loaded as a dev-mode package, a real login shell, and an actual `cd` typed into the terminal - verified by reading TerminalModel.cwd back out of the running app. This proves the one thing the headless jasmine specs can't: that the shipped shell scripts actually emit correct shell-integration sequences from a real shell startup, dotfiles and all. e2e/helpers.js launches Pulsar via Playwright's Electron support, with this checkout dev-linked in via a symlink under $ATOM_HOME/dev/packages (replicating what `ppm link --dev` does) so the real, current build is what activates rather than whatever `terminal` version Pulsar ships bundled. The workflow (.github/workflows/e2e.yml) runs the fast headless spec suite first so a broken spec fails the job before paying for the slower, more flake-prone E2E run, then runs the E2E suite itself across a strategy.matrix of bash/zsh/fish/pwsh on ubuntu-latest. Shell selection needs no test-side wiring at all: getDefaultShell() (src/config.ts) already reads $SHELL, and openPulsar() already spreads ...process.env into the launched app, so the workflow just resolves each shell's absolute path and exports it. zsh needs one extra step: a fresh `apt` install leaves its completion directories writable, which trips compinit's security check into an interactive prompt that swallows whatever's typed next - `compaudit` lists exactly which directories to fix. pwsh needs no install step at all, since GitHub's hosted ubuntu-latest runners ship it preinstalled. OS doesn't affect what this actually proves (the shell side of shell integration), so it isn't part of the matrix yet; the shell dimension is factored out on its own so growing the matrix further (fish/pwsh already here; macOS/Windows later) is just adding entries, not duplicating job definitions.
savetheclocktower
force-pushed
the
ci-playwright
branch
from
August 23, 2026 03:48
9736f6d to
b562f71
Compare
savetheclocktower
marked this pull request as ready for review
August 23, 2026 06:50
savetheclocktower
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I know this is a lot of code to review at once! Whenever I’ve thrown code at folks in the past, I’ve found that the reviewer is much more confident in their approval if the PR comes with a lot of tests.
I mentioned earlier how the shell integration was the one area that existing specs couldn’t get at. I knew this was something I could fix with Playwright in CI, and I knew that I should already have set up CI for this project, so the inertia I felt (because of the enormity of the task) was outweighed by my guilt.
I told Claude Code that I wanted to do this very devops-y thing, and described how I envisioned the Playwright end-to-end tests working. Eventually, it would be good to test all combinations of OS and shell (
fish,bash,zsh,pwsh), perhaps with certain holes in the matrix where appropriate. But that, in the short term, we were aiming to get justbash/Ubuntu working properly.This took forever for a few reasons:
terminalrepo was neverppm link’d into place.terminalpackage definesTerminalElement, then unconditionally registers it aspulsar-terminalviacustomElements.define. I didn’t realize this would be a problem… because I didn’t know that a core package always has its source code loaded (somehow) even if it’s not used because of package shadowing viappm link. So Pulsar complained as soon as the linked code also tried to definepulsar-terminal… because it was already defined.customElements.defineuntil activation. That’s easy to fix, but it won’t actually fix anything until the built-in version ofterminalgets that fix. So for now, I made it so that we fell back to a non-clashing element name ifpulsar-elementis taken. This also requires that we introduce some other reliable way of targeting this element via CSS selector (for styling, keymaps, menus, etc.), so I’ve got it adding adata-pulsar-terminalattribute in all cases. Once we ship 1.133.0 with this fix, we can go back to assumingpulsar-terminalwill be free. (As explained, all PRs in this stack got this fix — not just this one.)bash. It didn’t take much more effort to addzsh,fish, andpwshsupport in Ubuntu. Haven’t tackled the other platforms yet, but this demonstrates that the core mechanism for listening in oncwdchanges is sound for each supported shell.Once I got this working, I decided it was also worth running the unit tests as well. Adding that as a step was easy; but running the specs in a new environment revealed that they were much flakier in CI than they were on my machine. The process of diagnosing these flaky tests led to the discovery of a couple of subtle timing bugs; test reliability should be like 99% better now.
On the usage of Claude Code
I want to reassure anyone reading that they are not getting a glimpse into some sudden new LLM-maxxed development methodology on my part. This is a temporary mode I've put myself in:
hyperlinks, the bottom PR in this stack, was done entirely by hand, except that I eventually used Claude to help me write some of the specs that I had procrastinated on.So the rationales are different, but they all fall under the same umbrella: I considered all this work to be chores of various forms. I would find it quite disempowering and disheartening for this to be my main method of authoring code! But I also don't find it inherently edifying or character-building to write tests by hand, or to hand-author a GitHub Actions workflow file. I know myself well enough at this point to know that I could use some help with eating my vegetables.