Skip to content

Fix RealDataTestsBase cross-worktree test collisions#979

Open
johnml1135 wants to merge 3 commits into
mainfrom
fix/real-data-tests-worktree-isolation
Open

Fix RealDataTestsBase cross-worktree test collisions#979
johnml1135 wants to merge 3 commits into
mainfrom
fix/real-data-tests-worktree-isolation

Conversation

@johnml1135

@johnml1135 johnml1135 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • RealDataTestsBase (backing RenderBaselineTests, RenderBenchmarkTestsBase, and subclasses) resolved its scratch test-project directory via FwDirectoryFinder.ProjectsDirectory, a per-machine registry value that's correct for the shipping app (one shared Projects folder machine-wide) but means every git worktree of this repo collides on the same physical integration_test_data folder when running these tests.
  • Symptom: System.InvalidOperationException: Refusing to delete '...\integration_test_data' because the test sentinel file '.fieldworks-real-data-test-project' is missing — one worktree's test run deletes/rewrites another's project mid-run.
  • Fix (scoped entirely to this test fixture, FwDirectoryFinder's production API is untouched):
    1. DbDirectory() now resolves under this checkout's own SourceDirectory/../DistFiles/Projects, matching the pattern already used by other real-cache test fixtures in this repo (ConfiguredXHTMLGeneratorTests, RecordListTests, etc.).
    2. The project name is suffixed with a short hash of the worktree's SourceDirectory, because FwNewLangProjectModel.CheckForUniqueProjectName — correct, unrelated production logic for the real "New Project" wizard — checks name uniqueness against the shared ProjectsDirectory regardless of where this fixture's files live, so a fixed name still collided across worktrees. The suffix is stable within a worktree (preserving the mutex-guarded "reusable project" perf intent) but unique across worktrees.

Found and fixed while running CI locally as part of an unrelated PR #964 review — see that PR's PR_964_review.md §15 for the full investigation trail.

Test plan

  • RenderBaselineTests + RealDataTestsBaseCleanupTests (9 tests): went from 8/9 failing with the collision error to 9/9 passing.
  • Full ./test.ps1 CI-equivalent run

🤖 Generated with Claude Code


This change is Reviewable

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   10m 46s ⏱️ +7s
4 299 tests ±0  4 226 ✅ ±0  73 💤 ±0  0 ❌ ±0 
4 308 runs  ±0  4 235 ✅ ±0  73 💤 ±0  0 ❌ ±0 

Results for commit 4ad271e. ± Comparison against base commit 4509530.

♻️ This comment has been updated with latest results.

johnml1135 added a commit that referenced this pull request Jul 1, 2026
First of a 4-PR stack landing Phase 1 of the WinForms->Avalonia migration.
Every Avalonia surface is gated behind the UIMode setting, which DEFAULTS TO
"Legacy" (Src/Common/FwUtils/Properties/Settings.Designer.cs), so default
users see no behavioral change.

Contents:
- The Avalonia migration framework: region/composer (FullEntryRegionComposer),
  the typed view-definition IR compiled from XML layouts, owned dense controls,
  the seam contracts, and the region-editor plugin registry.
- The base detail-editor surfaces active under UIMode=New: lexiconEdit,
  lexiconEditPopup, notebookEdit, posEdit.
- The Avalonia browse table (LexicalBrowseView, LexicalBrowseHostControl,
  BulkEditBarView, ClerkBrowse*) and its RecordBrowseView product wiring,
  shipped DORMANT (fused to base wiring; activated by the table follow-up PR).
- 13 dialog UIs + a shared MessageBox riding behind UIMode=New, each verified
  wired to a real WinForms call site; 4 cleanly-removable unwired Avalonia
  dialogs backed out (SpecialCharacter, WritingSystemProperties,
  DeleteConfirmation, LexReferenceDetails).
- ChorusNotesBarControl (the Chorus/FLExBridge notes bar) rides as a Phase-1
  follow-up surface alongside the browse table and the interlinear/
  rule-formula plugins -- built, but not registered in this base PR.
- Migration skills/playbook (incl. the inert-surface activation recipe), the
  roadmap's core proposal/design/tasks/spec, and the landed openspec change
  specs (lexical-edit-avalonia-migration, shared-editable-virtualized-table,
  avalonia-multi-writing-system-text-foundation).

The interlinear and rule-formula detail editors are carved out to their own
stacked follow-up PRs; the inert tool lists are
LexicalEditSurfaceRegistry.Phase1FollowUpSurfaceTools and
LexicalEditSurfaceResolver.Phase1FollowUpBrowseTools. Migration per-screen
docs (the JIRA-ticket basis) and the full 13-stage migration-program planning
material live on the separate never-merged phase1-docs branch (the latter
also mirrored to chore/relocate-roadmap-planning-docs).

Phase 2 (avalonia-end-game: net multiplatform + shell conversion + WinForms
removal) is planned only and gated on Phase 1 + tester burn-down; its
proposal lives with the roadmap material above, not in this PR.

Post-review cleanup folded into this commit (see PR_964_review.md for the
full audit trail):
- Removed content unrelated to this migration that had been swept in:
  an unrelated installer-docs deletion and an ungated Legacy-mode rendering
  change were reverted, and a misplaced/stale stray doc file was dropped.
- Split two unrelated-but-legitimate fixes into their own PRs rather than
  carrying them here: #978 (VersionInfoProvider copyright/version-string
  bugs, a stale RegFree.targets entry, an opsx-prompt refactor) and #979
  (a pre-existing cross-worktree test-collision bug in RealDataTestsBase,
  found while verifying this branch but unrelated to it).
- Fixed a real build break (RecordBrowseView.cs used a pub/sub API signature
  main had already replaced) and a real product bug (the Avalonia refresh
  controller could stay unwired when a tool loads directly into UIMode=New,
  fixed in RecordEditView.cs).
- Pared back ~7,500 lines of speculative future-phase openspec planning
  docs (the 13-stage migration program, legacy-screenshot-capture tooling,
  the end-game proposal) to a separate branch, and deleted two fully
  superseded proposals and stale DataTree-model-view-separation specs
  asserting an architecture that was never built.
- Wired the one missing UIMode gate (GoLinkEntryDlgListener.OnGotoLexEntry),
  documented one honest parity deferral (MsaInflectionFeatureListDlgLauncher
  switch-tools navigation), removed dead API and orphaned localization keys,
  strengthened the FwAvalonia engine-isolation audit to match its own
  documented symbol list, fixed three evidence-language issues in
  Path3BundleTests, and reconciled dialog spacing tokens against real
  measured WinForms control geometry.

Verification: whole-solution build green. Surface-registry census: 6
custom-slice classes classified (LexemeEditorBurnDownTests); 7 tools in
LexicalEditSurfaceRegistry.Phase1FollowUpSurfaceTools; 8 tools in
LexicalEditSurfaceResolver.Phase1FollowUpBrowseTools. Full CI-equivalent test
run: all tests pass except 38 pre-existing, environment-specific
RealDataTestsBase cross-worktree-collision failures (fix is PR #979, kept
separate) and one test-harness limitation in RecordEditViewSwitchTests
documented in PR_964_review.md (the underlying product bug is fixed; the
test's idle-queue draining has a separate, deeper, pre-existing issue that
needs its own follow-up).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
johnml1135 added a commit that referenced this pull request Jul 2, 2026
First of a 4-PR stack landing Phase 1 of the WinForms->Avalonia migration.
Every Avalonia surface is gated behind the UIMode setting, which DEFAULTS TO
"Legacy" (Src/Common/FwUtils/Properties/Settings.Designer.cs), so default
users see no behavioral change.

Contents:
- The Avalonia migration framework: region/composer (FullEntryRegionComposer),
  the typed view-definition IR compiled from XML layouts, owned dense controls,
  the seam contracts, and the region-editor plugin registry.
- The base detail-editor surfaces active under UIMode=New: lexiconEdit,
  lexiconEditPopup, notebookEdit, posEdit.
- The Avalonia browse table (LexicalBrowseView, LexicalBrowseHostControl,
  BulkEditBarView, ClerkBrowse*) and its RecordBrowseView product wiring,
  shipped DORMANT (fused to base wiring; activated by the table follow-up PR).
- 13 dialog UIs + a shared MessageBox riding behind UIMode=New, each verified
  wired to a real WinForms call site; 4 cleanly-removable unwired Avalonia
  dialogs backed out (SpecialCharacter, WritingSystemProperties,
  DeleteConfirmation, LexReferenceDetails).
- ChorusNotesBarControl (the Chorus/FLExBridge notes bar) rides as a Phase-1
  follow-up surface alongside the browse table and the interlinear/
  rule-formula plugins -- built, but not registered in this base PR.
- Migration skills/playbook (incl. the inert-surface activation recipe), the
  roadmap's core proposal/design/tasks/spec, and the landed openspec change
  specs (lexical-edit-avalonia-migration, shared-editable-virtualized-table,
  avalonia-multi-writing-system-text-foundation).

The interlinear and rule-formula detail editors are carved out to their own
stacked follow-up PRs; the inert tool lists are
LexicalEditSurfaceRegistry.Phase1FollowUpSurfaceTools and
LexicalEditSurfaceResolver.Phase1FollowUpBrowseTools. Migration per-screen
docs (the JIRA-ticket basis) and the full 13-stage migration-program planning
material live on the separate never-merged phase1-docs branch (the latter
also mirrored to chore/relocate-roadmap-planning-docs).

Phase 2 (avalonia-end-game: net multiplatform + shell conversion + WinForms
removal) is planned only and gated on Phase 1 + tester burn-down; its
proposal lives with the roadmap material above, not in this PR.

Post-review cleanup folded into this commit (see PR_964_review.md for the
full audit trail):
- Removed content unrelated to this migration that had been swept in:
  an unrelated installer-docs deletion and an ungated Legacy-mode rendering
  change were reverted, and a misplaced/stale stray doc file was dropped.
- Split two unrelated-but-legitimate fixes into their own PRs rather than
  carrying them here: #978 (VersionInfoProvider copyright/version-string
  bugs, a stale RegFree.targets entry, an opsx-prompt refactor) and #979
  (a pre-existing cross-worktree test-collision bug in RealDataTestsBase,
  found while verifying this branch but unrelated to it).
- Fixed a real build break (RecordBrowseView.cs used a pub/sub API signature
  main had already replaced) and a real product bug (the Avalonia refresh
  controller could stay unwired when a tool loads directly into UIMode=New,
  fixed in RecordEditView.cs).
- Pared back ~7,500 lines of speculative future-phase openspec planning
  docs (the 13-stage migration program, legacy-screenshot-capture tooling,
  the end-game proposal) to a separate branch, and deleted two fully
  superseded proposals and stale DataTree-model-view-separation specs
  asserting an architecture that was never built.
- Wired the one missing UIMode gate (GoLinkEntryDlgListener.OnGotoLexEntry),
  documented one honest parity deferral (MsaInflectionFeatureListDlgLauncher
  switch-tools navigation), removed dead API and orphaned localization keys,
  strengthened the FwAvalonia engine-isolation audit to match its own
  documented symbol list, fixed three evidence-language issues in
  Path3BundleTests, and reconciled dialog spacing tokens against real
  measured WinForms control geometry.

Verification: whole-solution build green. Surface-registry census: 6
custom-slice classes classified (LexemeEditorBurnDownTests); 7 tools in
LexicalEditSurfaceRegistry.Phase1FollowUpSurfaceTools; 8 tools in
LexicalEditSurfaceResolver.Phase1FollowUpBrowseTools. Full CI-equivalent test
run: all tests pass except 38 pre-existing, environment-specific
RealDataTestsBase cross-worktree-collision failures (fix is PR #979, kept
separate) and one test-harness limitation in RecordEditViewSwitchTests
documented in PR_964_review.md (the underlying product bug is fixed; the
test's idle-queue draining has a separate, deeper, pre-existing issue that
needs its own follow-up).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

@jasonleenaylor reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A commit comment that contains all the comments also present in the code is a bit much. Please make sure we commit a much more results focused comment rather than details of the solution and maybe shorten the code comments as well.

@jasonleenaylor made 2 comments.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).


Src/Common/RootSite/RootSiteTests/RealDataTestsBase.cs line 169 at r1 (raw file):

			// delete or rewrite another's mid-run). Anchor to this checkout's own DistFiles/Projects
			// instead - the same SourceDirectory-derived, registry-free pattern already used by other
			// real-cache test fixtures in this repo (e.g. ConfiguredXHTMLGeneratorTests, RecordListTests).

This comment is awefully long and wordy, wouldn't mind a pass at tightening it up.

@johnml1135 johnml1135 force-pushed the fix/real-data-tests-worktree-isolation branch from 7a61c4f to 3b954ba Compare July 6, 2026 20:02
@johnml1135

Copy link
Copy Markdown
Contributor Author

Cleaned up comments to be simpler.

@johnml1135

Copy link
Copy Markdown
Contributor Author

Code review (verified against the code at the PR head)

HIGH — DbDirectory()'s worktree-local repoint doesn't match where the project actually gets validated/created, so the fix doesn't close the collision it targets

RealDataTestsBase.cs:152-158 repoints DbDirectory() to a worktree-local path (Path.GetDirectoryName(FwDirectoryFinder.SourceDirectory)\DistFiles\Projects) instead of FwDirectoryFinder.ProjectsDirectory. But EnsureSafeProjectDirectory (line 346-353+), which every actual-creation path (GetProjectDirectory, called from TestSetup at line 82 right after CreateNewLangProj) is validated against, still hardcodes the expected directory as:

var expectedProjectDirectory = NormalizePath(
    Path.Combine(FwDirectoryFinder.ProjectsDirectory, ReusableProjectName));

FwDirectoryFinder.ProjectsDirectory resolves from a machine-wide registry value (or %CommonApplicationData%\SIL\FieldWorks\Projects by default) — it can't be per-worktree. So the actual project created by CreateNewLangProj still lands under the shared, machine-wide ProjectsDirectory (that's what the validation in EnsureSafeProjectDirectory requires it to do, or GetProjectDirectory throws). The new worktree-suffixed ReusableProjectName (from the other hunk) does prevent same-name collisions across worktrees at that shared location, so that part is genuinely useful. But DbDirectory()'s worktree-local rerouting is now dead for the creation path and only affects the pre-create cleanup call at line 51 (m_projectDirectory = DbDirectory(m_dbName); ... DeleteProjectDirectory(m_projectDirectory) before CreateNewLangProj runs).

Concretely: if a prior test run crashes mid-setup and leaves an orphaned project directory at the real location (ProjectsDirectory\integration_test_data_<hash>), the next run's pre-create cleanup checks the wrong (worktree-local, never-populated) path and won't find or delete it — CreateNewLangProj then runs against a dirty leftover directory instead of a clean one.

Suggest either dropping the DbDirectory() change (the worktree suffix on ReusableProjectName already does the collision-avoidance work) or making EnsureSafeProjectDirectory's expected path consistent with DbDirectory()'s.

Reviewed with Claude Code; findings verified against the code at the PR head. No code changed.

@johnml1135

Copy link
Copy Markdown
Contributor Author

Fixed in 19dc4a3. Traced it further: CreateNewLangProjLcmCache.CreateNewLangProj always creates the project under dirs.ProjectsDirectory (ILcmDirectories.ProjectsDirectory, which is just FwDirectoryFinder.ProjectsDirectory) — it can't be redirected to a worktree-local path without deeper plumbing than this class has. So EnsureSafeProjectDirectory's hardcoded expectation was actually the correct/necessary one; DbDirectory() was the one that had drifted.

Reverted DbDirectory() to FwDirectoryFinder.ProjectsDirectory. The collision-avoidance this PR is actually after still holds: ReusableProjectName's worktree-hash suffix gives each worktree a distinct subdirectory name (and mutex name) under that same shared directory, so parallel worktrees no longer collide — and now the pre-create cleanup path checks the same location the project actually gets created/validated against.

@github-actions

This comment has been minimized.

johnml1135 and others added 2 commits July 7, 2026 16:01
CreateNewLangProj creates projects under FwDirectoryFinder.ProjectsDirectory
(via ILcmDirectories) and can't be redirected to a worktree-local path.
DbDirectory()'s worktree-local repoint therefore diverged from
EnsureSafeProjectDirectory's expectation, so the pre-create cleanup step
checked the wrong location and could miss orphaned data left by a crashed
prior run. Cross-worktree collision avoidance is already handled by
ReusableProjectName's worktree-hash suffix, which gives each worktree a
distinct subdirectory name (and mutex name) under the same shared
ProjectsDirectory.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@johnml1135 johnml1135 force-pushed the fix/real-data-tests-worktree-isolation branch from c628822 to 4ad271e Compare July 7, 2026 20:02
johnml1135 added a commit that referenced this pull request Jul 8, 2026
Combines 5 open dependabot PRs into one branch so they build and test
together instead of as 5 separate merges: #997, #984, #982, #981, #980.

Included bumps:
- Microsoft.Build.Utilities.Core 18.6.3 -> 18.7.1 (+5 others) (#997)
- nuget-minor group (10 updates): System.Drawing.Common,
  System.Reflection.Metadata, System.Resources.Extensions,
  System.Security.Permissions -> 9.0.17/10.0.9; SilLcm/SilLibPalaso
  version refresh in Build/SilVersions.props (#984)
- actions/checkout 6 -> 7 across all workflow files (#982)
- actions/cache 5 -> 6 in copilot-setup-steps.yml (#981)
- actions-minor group: 2 SHA-pinned action bumps in CI.yml and
  base-installer-cd.yml (#980)

Also pulls in #979's fix for RealDataTestsBase resolving its scratch
project directory via a per-machine shared path, which collided across
git worktrees and was needed to get a clean local test run.

Holds Microsoft.Extensions.DependencyModel at 9.0.16 instead of the
9.0.17 that #984 proposed: Directory.Packages.props has a long-standing
comment warning that bumping this package breaks ICU initialization in
the .NET Framework test host, and the full local test suite reproduced
that exact failure (TypeInitializationException on Icu.NativeMethods,
FileLoadException loading Microsoft.Extensions.DependencyModel) until
this one package was reverted. Every other package in that group is
kept at its bumped version.

Verified with build.ps1 -BuildTests (0 errors/warnings) and test.ps1
using the same TestFilter as CI.yml (4298 tests, only 12 failures, all
pre-existing local RenderBaselineTests/RenderBenchmark pixel-diff
mismatches unrelated to this change).
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