Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions scripts/dependents_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,33 @@
imports = []
file_dir = os.path.dirname(rel_path)

# ES imports
for m in re.finditer(r'import\s+.*?from\s+["\']([^"\']+)["\']', content):
# Issue #189: previous regex used `.*?` which doesn't match newlines,
# so multi-line imports like `import {\n foo,\n} from './bar'` were
# silently skipped. This left reverse_graph incomplete, which made
# `codelens affected` return affected_count: 0 even when real
# dependents existed. Use `[\s\S]*?` to match across newlines.
#
# ES imports (with from) — handles single-line AND multi-line forms.
for m in re.finditer(r'import\s+[\s\S]*?from\s+["\']([^"\']+)["\']', content):

Check warning on line 581 in scripts/dependents_engine.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Simplify this regular expression to reduce its runtime, as it has super-linear performance due to backtracking.

See more on https://sonarcloud.io/project/issues?id=Wolfvin_CodeLens&issues=AZ8ogFp-WEqI-IOmojNp&open=AZ8ogFp-WEqI-IOmojNp&pullRequest=192
resolved = _resolve_relative_import(m.group(1), file_dir, workspace)
if resolved:
imports.append(resolved)

# Issue #189: side-effect imports — `import './polyfills'` (no `from`).
# Must run AFTER the `from` form so we don't double-resolve, but the
# regex is disjoint: it requires a quote immediately after `import\s+`,
# which the `{ ... } from` form cannot match.
for m in re.finditer(r'import\s+["\']([^"\']+)["\']', content):
resolved = _resolve_relative_import(m.group(1), file_dir, workspace)
if resolved:
imports.append(resolved)

# Issue #189: re-exports — `export { x } from './bar'`,
# `export * from './bar'`, `export type { Foo } from './bar'`.
# These create a real dependency edge (the re-exporting module
# depends on the source module), so the reverse graph must include
# them or `codelens affected` will miss downstream consumers.
for m in re.finditer(r'export\s+[\s\S]*?from\s+["\']([^"\']+)["\']', content):

Check warning on line 600 in scripts/dependents_engine.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Simplify this regular expression to reduce its runtime, as it has super-linear performance due to backtracking.

See more on https://sonarcloud.io/project/issues?id=Wolfvin_CodeLens&issues=AZ8ogFp-WEqI-IOmojNq&open=AZ8ogFp-WEqI-IOmojNq&pullRequest=192
resolved = _resolve_relative_import(m.group(1), file_dir, workspace)
if resolved:
imports.append(resolved)
Expand Down
205 changes: 205 additions & 0 deletions tests/test_affected_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -621,3 +621,208 @@ def test_ts_no_workspace_arg_uses_cwd(self, ts_workspace):
assert result["workspace"] == os.path.abspath(ts_workspace)
assert "auth/google-auth-cache.ts" in result["changed_files"]
assert result["unresolved"] == []


# ─── Issue #189: BFS never starts when import parser misses TS patterns ──
#
# PR #184 fixed the workspace-as-first-arg heuristic so the existing
# ts_workspace fixture (single-line `import { x } from './y'`) works. But
# `_parse_js_imports` still missed three common TS/JS import shapes:
#
# 1. Multi-line imports — `import {\n foo,\n} from './bar'`
# Root cause: regex used `.*?` which doesn't cross newlines.
# 2. Side-effect imports — `import './polyfills'` (no `from` clause).
# Root cause: regex required `from`.
# 3. Re-exports — `export { x } from './bar'`, `export * from './bar'`.
# Root cause: regex only matched `import`, not `export ... from`.
#
# When the parser misses an import, the reverse_graph loses an edge. If the
# changed file's only inbound edge was the missed import, BFS finds no
# dependents and `affected_count` stays at 0 — even though real dependents
# exist. This is the "BFS never starts" symptom described in issue #189.


class TestIssue189ImportParserPatterns:
"""Regression tests for issue #189: `_parse_js_imports` must handle
multi-line imports, side-effect imports, and re-exports — otherwise
`_build_import_graph` produces an incomplete reverse_graph and
`codelens affected` returns ``affected_count: 0`` even when real
dependents exist.
"""

@pytest.fixture
def ts_workspace_multiline(self, tmp_path):
"""TS workspace where login.ts uses a multi-line import statement.

Mirrors the existing `ts_workspace` fixture but exercises the
multi-line import shape that real-world TS code uses pervasively
(Prettier default, ESLint `multi-line` rule, etc.).
"""
ws = tmp_path / "ws_multiline"
ws.mkdir()
(ws / "auth").mkdir()
(ws / "tests").mkdir()
(ws / "auth" / "google-auth-cache.ts").write_text(
"export const cache = new Map<string, string>();\n"
"export function getCachedToken(key: string): string | null {\n"
" return cache.get(key) || null;\n"
"}\n",
encoding="utf-8",
)
(ws / "auth" / "login.ts").write_text(
"import {\n"
" getCachedToken,\n"
"} from './google-auth-cache';\n"
"export function login(user: string): boolean {\n"
" const token = getCachedToken(user);\n"
" return token !== null;\n"
"}\n",
encoding="utf-8",
)
(ws / "tests" / "login.test.ts").write_text(
"import { login } from '../auth/login';\n"
"test('login returns false for unknown user', () => {\n"
" expect(login('unknown')).toBe(false);\n"
"});\n",
encoding="utf-8",
)
return str(ws)

@pytest.fixture
def ts_workspace_side_effect(self, tmp_path):
"""TS workspace where app.ts has a side-effect import (no `from`)."""
ws = tmp_path / "ws_side_effect"
ws.mkdir()
(ws / "auth").mkdir()
(ws / "tests").mkdir()
(ws / "auth" / "polyfills.ts").write_text(
"export const POLYFILL_VERSION = '1.0.0';\n",
encoding="utf-8",
)
(ws / "auth" / "app.ts").write_text(
"import './polyfills';\n"
"export function app(): string { return 'ok'; }\n",
encoding="utf-8",
)
(ws / "tests" / "app.test.ts").write_text(
"import { app } from '../auth/app';\n"
"test('app works', () => { expect(app()).toBe('ok'); });\n",
encoding="utf-8",
)
return str(ws)

@pytest.fixture
def ts_workspace_reexport(self, tmp_path):
"""TS workspace using `export { x } from './core'` (barrel file)."""
ws = tmp_path / "ws_reexport"
ws.mkdir()
(ws / "auth").mkdir()
(ws / "tests").mkdir()
(ws / "auth" / "core.ts").write_text(
"export const TOKEN_KEY = 'cl_token';\n",
encoding="utf-8",
)
(ws / "auth" / "index.ts").write_text(
"export { TOKEN_KEY } from './core';\n",
encoding="utf-8",
)
(ws / "tests" / "index.test.ts").write_text(
"import { TOKEN_KEY } from '../auth/index';\n"
"test('key is string', () => { expect(typeof TOKEN_KEY).toBe('string'); });\n",
encoding="utf-8",
)
return str(ws)

def test_multiline_import_resolves_and_bfs_finds_dependents(
self, ts_workspace_multiline,
):
"""Multi-line `import {\n foo \n} from './bar'` must be parsed.

Without the fix, `_parse_js_imports` skipped this import (regex
`.*?` did not cross newlines), `reverse_graph` had no edge from
`google-auth-cache.ts` to `login.ts`, and `affected_count` was 0.
"""
result = get_affected_files(
changed_files=["auth/google-auth-cache.ts"],
workspace=ts_workspace_multiline,
depth=5,
)
assert result["status"] == "ok"
assert result["unresolved"] == [], (
"changed file must resolve; unresolved means graph keys are "
"incomplete (the BFS-never-starts symptom from issue #189)"
)
assert result["stats"]["affected_count"] >= 1, (
"BFS must find the transitive test dependent via the multi-line "
"import in login.ts"
)
assert "tests/login.test.ts" in result["affected"]

def test_side_effect_import_resolves_and_bfs_finds_dependents(
self, ts_workspace_side_effect,
):
"""`import './polyfills'` (no `from`) must be parsed as an edge."""
result = get_affected_files(
changed_files=["auth/polyfills.ts"],
workspace=ts_workspace_side_effect,
depth=5,
)
assert result["status"] == "ok"
assert result["unresolved"] == []
assert result["stats"]["affected_count"] >= 1, (
"side-effect imports create a real dependency edge — BFS must "
"find the transitive test dependent"
)
assert "tests/app.test.ts" in result["affected"]

def test_reexport_creates_dependency_edge(
self, ts_workspace_reexport,
):
"""`export { x } from './core'` must add core.ts → index.ts edge."""
result = get_affected_files(
changed_files=["auth/core.ts"],
workspace=ts_workspace_reexport,
depth=5,
)
assert result["status"] == "ok"
assert result["unresolved"] == []
assert result["stats"]["affected_count"] >= 1, (
"re-exports create a real dependency edge — BFS must find the "
"transitive test dependent via the barrel file"
)
assert "tests/index.test.ts" in result["affected"]

def test_existing_ts_workspace_fixture_still_finds_affected(self, ts_workspace):
"""DoD sanity check: the existing fixture from PR #184 must still
return ``affected_count >= 1`` after the parser changes.

This is the explicit Definition-of-Done assertion from issue #189:
"Use existing ts_workspace fixture in tests/test_affected_command.py
(from PR #184) and assert affected_count >= 1".
"""
result = get_affected_files(
changed_files=["auth/google-auth-cache.ts"],
workspace=ts_workspace,
depth=5,
)
assert result["status"] == "ok"
assert result["unresolved"] == []
assert result["stats"]["affected_count"] >= 1
assert "tests/login.test.ts" in result["affected"]

def test_multiline_include_source_returns_all_dependents(
self, ts_workspace_multiline,
):
"""`--include-source` must also surface non-test dependents reached
via multi-line imports (cross-check against the existing
`test_ts_include_source_returns_all_dependents`).
"""
result = get_affected_files(
changed_files=["auth/google-auth-cache.ts"],
workspace=ts_workspace_multiline,
depth=5,
include_source=True,
)
affected = set(result["affected"])
assert "auth/login.ts" in affected
assert "tests/login.test.ts" in affected
Loading