Skip to content

Decouple PHP integrations from the shared core - #116

Draft
revenant20 wants to merge 7 commits into
j-plugins:open-idefrom
revenant20:shared-core
Draft

revenant20 wants to merge 7 commits into
j-plugins:open-idefrom
revenant20:shared-core

Conversation

@revenant20

Copy link
Copy Markdown

Related to #115.

Shared Testo features currently reference PhpStorm's PHP APIs directly. Introduce PHP and tool-environment contracts, move implementation-specific classes and registrations to src/phpstorm, and retain the existing 252/262 builds and saved configuration formats.

Project guidance is maintained in CLAUDE.md, with AGENTS.md forwarding to it.

The shared code owns run selections, command arguments, navigation hints, coverage and mutation lifecycle. Contract tests, behavior snapshots and an isolation check cover the boundary. The changes also make data-provider ordering deterministic, keep temporary rerun filters out of saved configurations, preserve stopped mutation reruns and protect active archives during cleanup. Test fixtures normalize temporary paths on macOS.

Validation on 0dac21972c4a76bec3144dea2e2b4842a356c1ad: check buildPlugin passed on both 252 and 262, with 728 tests per variant and no failures or skips. The packaged contents match the builds verified as compatible with IU 252.28539.97, 253.33813.55, 261.27258.48, 262.10968.63 and 263.6259.32. Plugin Verifier reports deprecated, scheduled-for-removal and experimental API usages, but no compatibility problems.

Base: upstream main at 0b8c04e47c21dabcae4143c9e0c1bb3d12d3fc01.

The runtime sources, tests and build configuration at 449feb246eb2bd0e7b0103cfafbc7ad1245db9ac are unchanged from the tested revision 0dac21972c4a76bec3144dea2e2b4842a356c1ad. The documentation update was checked for links and consistency with the source layout.

@revenant20 revenant20 mentioned this pull request Oct 3, 2026
@roxblnfk
roxblnfk changed the base branch from main to open-ide October 8, 2026 15:55
…launch

refactor(run): drop the unused toRemoteIfMapped helper
test: compare local paths the way the host spells them
docs: point CLAUDE.md at TestoLocationHints, groupNamesOf and the tool environment

The local path processor returns OS-native separators, so the two tests failed on Windows while passing on Linux CI.

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>

@roxblnfk roxblnfk left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this — it is a large piece of work, and the move itself holds up. I compared main against this branch area by area: the producer, the PSI predicates and indexes, line markers and location hints, the command line and coverage flags, the descriptors and every persisted id. Those are equivalent, down to the bytecode where the PHP API is involved; no index version needs a bump, and no saved id or XML name changed.

What blocks it are behaviour changes riding along with the refactor, inline below. Still open: archived runs that can no longer be deleted, a mutation lock that survives a restart, and a deletion that holds the archive lock the EDT waits on. These look like groundwork for #117's remote-process guarantees, so they want your call on the policy rather than a revert to main. (The rerun-filter comment was wrong and is retracted in its thread.)

Two notes on the tests:

  • The baselines are recorded from the refactored code: they arrive in the same commit and import com.github.xepozz.testo.phpstorm.*, so they cannot run on main. They pin future behaviour, but they do not show that this refactor preserves main's. They also cover no remote interpreter, no clone(), no rerunFilters and no Debug run, so none of the regressions below trips them. Splitting would make the claim checkable: the baselines against main first, then the move with the baseline files untouched, then each fix on its own.
  • On Windows, 8 baseline tests fail: local paths come back with \ (the platform's own setters and local path processor, as on main), and CommandBaselineTest stands in for php with a /bin/sh script. CI runs on Linux, so this is low priority.

I pushed two commits with the small things:

  • d937227: the missing-interpreter error goes through the bundle again (infection.error.noInterpreter had lost its last user), the unused toRemoteIfMapped is gone, two tests compare local paths the way the host spells them, and CLAUDE.md points at TestoLocationHints.parse, groupNamesOf and the tool environment instead of the removed names.
  • d0c4f64: the Coverage view's Mutate resolves the environment off the EDT, Infection starts without the Testo run's (possibly Debug) environment, and Apply/Revert are offered only where they can write. Both merge cleanly into #117; its only conflict, gradle/libs.versions.toml, comes from the earlier merge of main.


private fun mustKeep(dir: Path): Boolean {
if (hasInputUsers(dir)) return true
val state = summary(dir) ?: return true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A mutation directory without a readable mutation.json is kept forever, and through deleteRunIfSafe it now pins its whole Testo run: TestoRunStore.delete ignores the false, so retention, Discard and Clear history skip the run silently.

Such directories already exist on users' disks. On main the summary is written only on success (recorder.summary(run) inside use, no finally), so a mutation run that failed to start (no infection binary, say) left infection/<ts>/ with just stream.log. After the update those Testo runs can never be deleted. With this PR the same happens whenever the IDE dies mid-run, before the finally writes the summary.

TestoRunStore gives a manifest-less run a 24 h grace for the same situation. Something similar here (protect a summary-less directory only while it has input users or is tracked, or only within a grace period) would keep the protection without the leak.

internal fun start(recipe: TestoMutationRecipe) {
val runDir = recipe.runDir
runs[runDir]?.takeIf { it.isBusy }?.let { return TestoMutationToolWindow.show(project, it) }
TestoMutationArchive.unconfirmedRun(runDir)?.let { dir ->

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

unconfirmedReason is persisted, and only releaseTool clears it, which needs the live handler to report termination later. After an IDE restart nothing can do that:

  • every Mutate for this Testo run just reopens the restored run, which stays isBusy for good (TestoMutationModel.kt:196), so rerun is disabled;
  • Stop is disabled too, since stopper is null;
  • mustKeep keeps the archive away from retention and Clear history.

The infection.error.unconfirmedRestored text asks the user to check the processes before starting another run, but there is no way to start one, short of deleting the directory by hand.

The path there: Stop on an SSH or Docker interpreter whose handler does not terminate within the 5 s + 5 s window, then close the IDE. A restored unconfirmed run probably needs an explicit "checked, continue" action, or should stop blocking once the session that owned the process is gone.

Comment thread src/main/kotlin/com/github/xepozz/testo/coverage/TestoCoverageMutation.kt Outdated
}.getOrDefault(true)
if (protected) return false
}
NioFiles.deleteRecursively(sourceRunDir)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The whole Testo run directory, coverage reports included, is deleted while holding the TestoMutationArchive monitor. Retention and Clear history run this on a pooled thread, and TestoMutationService.start takes the same monitor on the EDT (unconfirmedRun, prune), so clicking Mutate during a cleanup freezes the UI for as long as the deletion takes.

Deciding under the lock and deleting outside it (e.g. renaming the directory to a tombstone under the lock first) would keep check-and-act atomic without the stall.

Comment thread src/main/kotlin/com/github/xepozz/testo/infection/TestoMutationApply.kt Outdated
Comment thread CHANGELOG.md Outdated
…the EDT

fix(infection): start Infection without the Testo run's environment
fix(infection): offer Apply and Revert only where they can write
docs: describe the mutation launch independently of the PHP implementation

A Debug run's environment carries its Xdebug session into every mutant process.

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants