mirror of
https://github.com/firstsun-dev/git-files-sync.git
synced 2026-07-22 17:20:30 +00:00
test: add SyncStatusView coverage for the delete-path and folder-collision fixes
Adds minimal ItemView/WorkspaceLeaf mocks to tests/setup.ts (the shared 'obsidian' module mock) -- SyncStatusView had no test file before this. New tests/ui/SyncStatusView.test.ts covers: - performRemoteDeletion strips the vaultFolder prefix before calling gitService.deleteFile (with and without vaultFolder configured), and records the real error message instead of swallowing it. - identifyExtraFiles classifies a local folder colliding with a stale remote record as remote-only rather than a readable file, while still treating a genuine local file as checkable.
This commit is contained in:
parent
fa42fea5fd
commit
f1420f0b44
3 changed files with 172 additions and 3 deletions
|
|
@ -27,9 +27,10 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont
|
|||
1. **URL encoding + silent empty-sha delete**: `github-service.ts`/`gitea-service.ts` `getApiUrl()` built the Contents API URL without `encodeURIComponent` on path segments (unlike `gitlab-service.ts`, which already encodes), so paths with spaces/non-ASCII (e.g. Chinese filenames) 404'd on the pre-delete sha lookup; that 404 silently produced an empty `sha`, and the resulting DELETE with `sha: ''` was rejected — silently, since the UI swallowed all delete errors. Fixed: encode path segments in both services; `deleteFile()` now throws a clear error if the pre-delete lookup's sha is empty; `SyncStatusView.ts` delete flow now captures `{path, message}` per failure and surfaces the real message in the Notice (`syncStatus.notice.deleteResult.partialWithMessage` i18n key, added to both `en.ts`/`zh-tw.ts`) instead of a bare "N failed".
|
||||
2. **EISDIR + folder/remote-record collision**: `SyncStatusView.ts` `readFileContent()`/`identifyExtraFiles()` treated any local path that `adapter.exists()` returned true for as a readable file — but a real local directory (or a symlink to one) that happens to share a path with a stale remote record (e.g. `.claude/skills/polish-blog`) is not readable content, and `adapter.read()` on it throws `EISDIR`. Fixed: new `isLocalFile()` helper (`adapter.stat().type === 'file'`) gates `identifyExtraFiles` and the hidden-file `recursiveScan`; `readFileContent`'s string-path branch (extracted to `readStringPathContent()`) also falls back to `readLocalSymlinkTarget()` on read failure as a last resort.
|
||||
3. **Wrong path passed to deleteFile (the actual reported bug)**: `performRemoteDeletion` called `gitService.deleteFile(s.path, ...)` with `s.path` — a *vault-relative* path (carries the `vaultFolder` prefix, per `identifyExtraFiles`'s `getVaultPath()` mapping) — but `deleteFile`'s `getFullPath()` expects a path relative to `rootPath` only (vaultFolder is a purely local concept, stripped before every other `gitService` call site, e.g. `sync-manager.ts`'s `getNormalizedPath(file.path)` pattern used everywhere else). With a non-empty `vaultFolder` setting, this built the wrong repo path, causing `deleteFile`'s pre-delete `getFile()` lookup to 404 — surfacing as "file was not found on branch main" for a file the UI still listed as remote-only (since refresh's status computation *does* normalize correctly). Fixed: `performRemoteDeletion` now computes `this.plugin.getNormalizedPath(s.path)` before calling `deleteFile`, matching every other call site.
|
||||
- Added 4 new tests (2 in `tests/services/github-service.test.ts`, 2 in `tests/services/gitea-service.test.ts`) covering the empty-sha throw and non-ASCII/space path encoding. No new test yet for root cause #3 (vaultFolder/rootPath path mismatch) or #2 (folder collision) — `SyncStatusView.ts` currently has no dedicated test file; consider adding one if this recurs.
|
||||
- Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 312/312 passed.
|
||||
- Root causes #1+#2 committed as `896d77b`, merged with the i18n branch (`563a28e`), pushed. Root cause #3 fixed after that push, in this same session — **not yet committed/pushed as of this note**.
|
||||
- Added 4 tests for root cause #1 (`tests/services/github-service.test.ts`, `tests/services/gitea-service.test.ts`) covering the empty-sha throw and non-ASCII/space path encoding.
|
||||
- Added `tests/ui/SyncStatusView.test.ts` (5 cases) for root causes #2 and #3, since `SyncStatusView.ts` had no test file at all before this — required adding minimal `ItemView`/`WorkspaceLeaf` mocks to `tests/setup.ts` (constructor sets `app`/`leaf`, fakes `containerEl.children[0..1]`; no rendering behavior needed since tests call private methods directly via a cast rather than going through `onOpen()`/`renderView()`). Covers: vaultFolder-prefix stripping before `deleteFile()` (with and without a configured vaultFolder), real error messages surfacing in `errors`, and `identifyExtraFiles` correctly classifying a local-folder/remote-record collision as remote-only vs. a genuine local file as checkable.
|
||||
- Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 317/317 passed.
|
||||
- All three root causes committed (`896d77b`, merge `563a28e`, `fa42fea`) and pushed to `claude/fix-directory-symlink-pull-260713`. Test coverage for #2/#3 added after that — pending commit as of this note.
|
||||
- Not yet done: comment on/reference issue #78 (in the `blog` repo) noting the fix landed in `git-files-sync` instead — flag to user before doing so since it crosses repos.
|
||||
|
||||
### What's In Progress
|
||||
|
|
|
|||
|
|
@ -211,6 +211,27 @@ export const Modal = class {
|
|||
|
||||
export const MarkdownView = class {};
|
||||
export const Editor = class {};
|
||||
export const WorkspaceLeaf = class {};
|
||||
export const ItemView = class {
|
||||
app: unknown;
|
||||
leaf: unknown;
|
||||
containerEl: HTMLElement;
|
||||
|
||||
constructor(leaf?: { app?: unknown }) {
|
||||
this.leaf = leaf;
|
||||
this.app = leaf?.app;
|
||||
// Real ItemView's containerEl has a header at children[0] and the
|
||||
// content root at children[1]; views render into the latter.
|
||||
this.containerEl = document.createElement('div') as HTMLElement;
|
||||
this.containerEl.appendChild(document.createElement('div'));
|
||||
this.containerEl.appendChild(document.createElement('div'));
|
||||
}
|
||||
|
||||
registerEvent() {}
|
||||
register() {}
|
||||
registerDomEvent() {}
|
||||
registerInterval() {}
|
||||
};
|
||||
export const App = class {
|
||||
workspace = {
|
||||
getActiveViewOfType: vi.fn(),
|
||||
|
|
@ -256,6 +277,8 @@ vi.mock('obsidian', () => ({
|
|||
Modal,
|
||||
MarkdownView,
|
||||
Editor,
|
||||
WorkspaceLeaf,
|
||||
ItemView,
|
||||
App,
|
||||
TFile,
|
||||
TFolder,
|
||||
|
|
|
|||
145
tests/ui/SyncStatusView.test.ts
Normal file
145
tests/ui/SyncStatusView.test.ts
Normal file
|
|
@ -0,0 +1,145 @@
|
|||
import { describe, it, expect, vi, beforeAll } from 'vitest';
|
||||
import { SyncStatusView } from '../../src/ui/SyncStatusView';
|
||||
import { WorkspaceLeaf, Notice } from 'obsidian';
|
||||
import type GitLabFilesPush from '../../src/main';
|
||||
import { setupObsidianDOM } from './setup-dom';
|
||||
import type { FileStatus } from '../../src/ui/types';
|
||||
import type { GitTreeEntry } from '../../src/services/git-service-interface';
|
||||
|
||||
// Minimal fake plugin: only the surface these tests actually exercise.
|
||||
function makePlugin(overrides: {
|
||||
vaultFolder?: string;
|
||||
deleteFile?: ReturnType<typeof vi.fn>;
|
||||
adapterExists?: ReturnType<typeof vi.fn>;
|
||||
adapterStat?: ReturnType<typeof vi.fn>;
|
||||
getAbstractFileByPath?: ReturnType<typeof vi.fn>;
|
||||
} = {}): { plugin: GitLabFilesPush; leaf: WorkspaceLeaf; deleteFile: ReturnType<typeof vi.fn> } {
|
||||
const vaultFolder = overrides.vaultFolder ?? '';
|
||||
const deleteFile = overrides.deleteFile ?? vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
const app = {
|
||||
vault: {
|
||||
adapter: {
|
||||
exists: overrides.adapterExists ?? vi.fn().mockResolvedValue(false),
|
||||
stat: overrides.adapterStat ?? vi.fn().mockResolvedValue(null),
|
||||
},
|
||||
getAbstractFileByPath: overrides.getAbstractFileByPath ?? vi.fn().mockReturnValue(null),
|
||||
},
|
||||
};
|
||||
|
||||
const plugin = {
|
||||
settings: { branch: 'main', vaultFolder },
|
||||
gitService: { deleteFile },
|
||||
getNormalizedPath(path: string): string {
|
||||
if (!vaultFolder) return path;
|
||||
const prefix = `${vaultFolder}/`;
|
||||
if (path.startsWith(prefix)) return path.substring(prefix.length);
|
||||
if (path === vaultFolder) return '';
|
||||
return path;
|
||||
},
|
||||
} as unknown as GitLabFilesPush;
|
||||
|
||||
const leaf = { app } as unknown as WorkspaceLeaf;
|
||||
return { plugin, leaf, deleteFile };
|
||||
}
|
||||
|
||||
describe('SyncStatusView remote deletion', () => {
|
||||
beforeAll(() => { setupObsidianDOM(); });
|
||||
|
||||
// Regression test for the bug where deleteFile() received the vault-relative
|
||||
// path (carrying the vaultFolder prefix) instead of the repo-relative path,
|
||||
// causing a spurious "file was not found on branch main" for files the UI
|
||||
// itself listed as remote-only.
|
||||
it('strips the vaultFolder prefix before calling gitService.deleteFile', async () => {
|
||||
const { plugin, leaf, deleteFile } = makePlugin({ vaultFolder: '02_Areas/blog' });
|
||||
const view = new SyncStatusView(leaf, plugin);
|
||||
|
||||
const fileStatus: FileStatus = { path: '02_Areas/blog/notes/todo.md', status: 'remote-only' };
|
||||
const errors: { path: string, message: string }[] = [];
|
||||
const prog = new Notice('', 0);
|
||||
|
||||
// performRemoteDeletion is private; called directly to isolate it from
|
||||
// the confirmation dialog and higher-level orchestration in deleteSelected().
|
||||
await (view as unknown as {
|
||||
performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: { path: string, message: string }[]): Promise<void>
|
||||
}).performRemoteDeletion([fileStatus], 1, 0, prog, errors);
|
||||
|
||||
expect(deleteFile).toHaveBeenCalledWith('notes/todo.md', 'main', expect.any(String));
|
||||
expect(errors).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('passes the path unchanged when no vaultFolder is configured', async () => {
|
||||
const { plugin, leaf, deleteFile } = makePlugin();
|
||||
const view = new SyncStatusView(leaf, plugin);
|
||||
|
||||
const fileStatus: FileStatus = { path: 'notes/todo.md', status: 'remote-only' };
|
||||
const errors: { path: string, message: string }[] = [];
|
||||
const prog = new Notice('', 0);
|
||||
|
||||
await (view as unknown as {
|
||||
performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: { path: string, message: string }[]): Promise<void>
|
||||
}).performRemoteDeletion([fileStatus], 1, 0, prog, errors);
|
||||
|
||||
expect(deleteFile).toHaveBeenCalledWith('notes/todo.md', 'main', expect.any(String));
|
||||
});
|
||||
|
||||
it('records the real error message instead of swallowing it', async () => {
|
||||
const deleteFile = vi.fn().mockRejectedValue(new Error('Cannot delete "notes/todo.md": file was not found on branch "main".'));
|
||||
const { plugin, leaf } = makePlugin({ deleteFile });
|
||||
const view = new SyncStatusView(leaf, plugin);
|
||||
|
||||
const fileStatus: FileStatus = { path: 'notes/todo.md', status: 'remote-only' };
|
||||
const errors: { path: string, message: string }[] = [];
|
||||
const prog = new Notice('', 0);
|
||||
|
||||
await (view as unknown as {
|
||||
performRemoteDeletion(remote: FileStatus[], total: number, localCount: number, prog: Notice, errors: { path: string, message: string }[]): Promise<void>
|
||||
}).performRemoteDeletion([fileStatus], 1, 0, prog, errors);
|
||||
|
||||
expect(errors).toEqual([{ path: 'notes/todo.md', message: 'Cannot delete "notes/todo.md": file was not found on branch "main".' }]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('SyncStatusView.identifyExtraFiles folder/remote-record collisions', () => {
|
||||
beforeAll(() => { setupObsidianDOM(); });
|
||||
|
||||
// Regression test: a local real directory (or a symlink to one) can share a
|
||||
// path with a stale remote record (e.g. a folder that used to be a pushed
|
||||
// symlink). Treating it as a readable file crashes adapter.read() with EISDIR;
|
||||
// it should be classified remote-only instead.
|
||||
it('treats a path that exists locally as a folder as remote-only, not a readable file', async () => {
|
||||
const adapterStat = vi.fn().mockResolvedValue({ type: 'folder' });
|
||||
const adapterExists = vi.fn().mockResolvedValue(true);
|
||||
const { plugin, leaf } = makePlugin({ adapterStat, adapterExists });
|
||||
const view = new SyncStatusView(leaf, plugin);
|
||||
|
||||
const remoteMap = new Map<string, GitTreeEntry>([
|
||||
['.claude/skills/polish-blog', { path: '.claude/skills/polish-blog', symlink: false }],
|
||||
]);
|
||||
|
||||
const extra = await (view as unknown as {
|
||||
identifyExtraFiles(remoteMap: Map<string, GitTreeEntry>, localFilePaths: Set<string>, allLocalFileMap: Map<string, unknown>): Promise<unknown[]>
|
||||
}).identifyExtraFiles(remoteMap, new Set(), new Map());
|
||||
|
||||
expect(extra).toEqual([]);
|
||||
const statuses = (view as unknown as { fileStatuses: Map<string, FileStatus> }).fileStatuses;
|
||||
expect(statuses.get('.claude/skills/polish-blog')).toEqual({ path: '.claude/skills/polish-blog', status: 'remote-only' });
|
||||
});
|
||||
|
||||
it('still treats a genuine local file as extra/checkable', async () => {
|
||||
const adapterStat = vi.fn().mockResolvedValue({ type: 'file' });
|
||||
const adapterExists = vi.fn().mockResolvedValue(true);
|
||||
const { plugin, leaf } = makePlugin({ adapterStat, adapterExists });
|
||||
const view = new SyncStatusView(leaf, plugin);
|
||||
|
||||
const remoteMap = new Map<string, GitTreeEntry>([
|
||||
['notes/hidden.md', { path: 'notes/hidden.md', symlink: false }],
|
||||
]);
|
||||
|
||||
const extra = await (view as unknown as {
|
||||
identifyExtraFiles(remoteMap: Map<string, GitTreeEntry>, localFilePaths: Set<string>, allLocalFileMap: Map<string, unknown>): Promise<unknown[]>
|
||||
}).identifyExtraFiles(remoteMap, new Set(), new Map());
|
||||
|
||||
expect(extra).toEqual(['notes/hidden.md']);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue