tests: init #29
tests: init #29
Conversation
WalkthroughAdds a CI GitHub Actions workflow, ESM test runner shim, Helm language metadata, expanded integration tests and Helm chart fixtures, a CI-only guard for an executable test suite, and TypeScript compiler option and exclude updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant VSX as VS Code Extension Host
participant LS as Helm Language Server
participant Doc as Helm Document
rect rgb(225,240,255)
Note over Test,VSX: Open Helm file -> trigger activation
Test->>VSX: openHelmDocument(workspaceFolder/templates/deployment.yaml)
activate VSX
VSX->>LS: initialize/activate for Helm YAML
LS-->>VSX: initialized
VSX-->>Test: extension activated
deactivate VSX
end
rect rgb(240,255,230)
Note over Test,LS: Hover interactions and assertions
Test->>Doc: request hover at position
Doc->>LS: hover request
LS-->>Doc: hover response (Helm/schema)
Doc-->>Test: hover content returned
Test->>Test: assert expected hover text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/test/extension.test.ts (5)
9-22: Make polling interval configurable; clearer timeout message.Expose poll interval and improve timeout wording to aid debugging and flakes.
-async function waitForExtensionActivation(extensionId: string, timeout: number = 10000): Promise<vscode.Extension<any>> { +async function waitForExtensionActivation( + extensionId: string, + timeout: number = 10000, + pollIntervalMs: number = 100 +): Promise<vscode.Extension<any>> { @@ - await new Promise(resolve => setTimeout(resolve, 100)); + await new Promise(resolve => setTimeout(resolve, pollIntervalMs)); } - throw new Error(`Extension ${extensionId} failed to activate within ${timeout}ms`); + throw new Error(`Extension "${extensionId}" not active after ${timeout}ms (not found or activation condition unmet)`); }
27-40: Avoid fixed 1s sleep; poll for server readiness via a hover probe.Static waits are flaky on CI. Prefer polling until a basic hover returns or timeout.
Add once per file near helpers:
async function waitForHover( uri: vscode.Uri, pos: vscode.Position, timeout = 15000, interval = 150 ): Promise<vscode.Hover[]> { const start = Date.now(); while (Date.now() - start < timeout) { const h = await vscode.commands.executeCommand<vscode.Hover[]>('vscode.executeHoverProvider', uri, pos); if (h && h.length > 0) return h; await new Promise(r => setTimeout(r, interval)); } throw new Error('Timed out waiting for hover results'); }Then remove the fixed sleep here, and let each test call waitForHover at the target position after opening.
75-82: Avoid hard-coded position; compute it from document text.Line/column guesses are brittle. Search the document for the token to build a Position.
- // Test hover on .Values.replicaCount (line 9, assuming it's around column 25) - const helmPosition = new vscode.Position(8, 25); + // Find ".Values.replicaCount" dynamically + const doc = await vscode.workspace.openTextDocument(docUri); + let helmPosition: vscode.Position | undefined; + for (let i = 0; i < doc.lineCount; i++) { + const text = doc.lineAt(i).text; + const idx = text.indexOf('.Values.replicaCount'); + if (idx >= 0) { helmPosition = new vscode.Position(i, idx + 1); break; } + } + assert.ok(helmPosition, 'Could not locate .Values.replicaCount in template');
106-113: Also compute YAML position dynamically.Search for the first occurrence of "spec:" to avoid line drift.
- // Test hover on 'spec' property (line 7) - const yamlPosition = new vscode.Position(6, 3); + // Find 'spec:' dynamically + const yamlDoc = await vscode.workspace.openTextDocument(docUri); + let yamlPosition: vscode.Position | undefined; + for (let i = 0; i < yamlDoc.lineCount; i++) { + const text = yamlDoc.lineAt(i).text; + const idx = text.indexOf('spec:'); + if (idx >= 0) { yamlPosition = new vscode.Position(i, Math.max(idx + 1, 3)); break; } + } + assert.ok(yamlPosition, "Couldn't locate 'spec:' in template");
52-62: Extract extension id into a constant to avoid typos and simplify future maintenance.The extension id
'helm-ls.helm-ls'appears three times in the test. Extract it as a module-level constant:+const EXT_ID = 'helm-ls.helm-ls'; + describe('Helm LS Extension Test Suite', function() { @@ - const extBefore = vscode.extensions.getExtension('helm-ls.helm-ls'); + const extBefore = vscode.extensions.getExtension(EXT_ID); @@ - const ext = await waitForExtensionActivation('helm-ls.helm-ls'); + const ext = await waitForExtensionActivation(EXT_ID); @@ - assert.strictEqual(ext.id, 'helm-ls.helm-ls'); + assert.strictEqual(ext.id, EXT_ID);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/extension.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/test/extension.test.ts (4)
85-88: Open document before waiting for activation.These tests assume prior activation. Opening the document first triggers the activation event (
onLanguage:helm), then waiting ensures each test is independent. The current order creates inter-test dependencies.Apply this diff:
- // Ensure extension is activated - await waitForExtensionActivation("helm-ls.helm-ls"); - const { uri: docUri } = await openHelmDocument(workspaceFolder); + await waitForExtensionActivation("helm-ls.helm-ls");
103-111: Robustly parse hover contents (avoid MarkdownString cast).Hover items can be
string,MarkdownString, orMarkedString. Casting directly toMarkdownStringand accessing.valuewill throw at runtime if the content is a plain string or different type.Add a helper function to safely extract text from all hover content types:
function flattenHoverText(hovers: vscode.Hover[]): string { const parts: string[] = []; for (const h of hovers ?? []) { for (const c of h.contents ?? []) { if (typeof c === 'string') { parts.push(c); continue; } const anyC = c as any; if (typeof anyC.value === 'string') { parts.push(anyC.value); } } } return parts.join('\n'); }Then replace the unsafe cast:
- const helmHoverContent = helmHovers[0].contents[0] as vscode.MarkdownString; - assert.ok(helmHoverContent, 'Hover content should exist'); - // Check for expected content (replicaCount value from values.yaml) - assert.ok( - helmHoverContent.value.includes("1") || - helmHoverContent.value.includes("replicaCount"), - `Hover should show replicaCount value or reference. Got: ${helmHoverContent.value}`, - ); + const helmText = flattenHoverText(helmHovers); + assert.ok(helmText.length > 0, 'Hover content should exist'); + assert.ok(helmText.includes('replicaCount') || helmText.includes('1'), + `Hover should show replicaCount or value. Got: ${helmText}`);
120-123: Open document before waiting for activation.Same ordering issue as the previous hover test. Open the document first to trigger activation, then wait.
Apply this diff:
- // Ensure extension is activated - await waitForExtensionActivation("helm-ls.helm-ls"); - const { uri: docUri } = await openHelmDocument(workspaceFolder); + await waitForExtensionActivation("helm-ls.helm-ls");
138-148: Robustly parse hover contents (avoid MarkdownString cast).Same unsafe casting issue as the previous hover test. Use the
flattenHoverTexthelper suggested above.Apply this diff:
- const yamlHoverContent = yamlHovers[0].contents[0] as vscode.MarkdownString; - assert.ok(yamlHoverContent, "YAML hover content should exist"); - - // Check for Kubernetes documentation - const content = yamlHoverContent.value.toLowerCase(); + const content = flattenHoverText(yamlHovers).toLowerCase(); assert.ok( content.includes("deployment") || content.includes("spec") || content.includes("specification"), - `Hover should show Kubernetes schema info. Got: ${yamlHoverContent.value}`, + `Hover should show Kubernetes schema info. Got: ${content}`, );
🧹 Nitpick comments (2)
src/test/extension.test.ts (2)
48-49: Replace fixed sleep with polling for language server readiness.The hardcoded 1000ms wait can cause flaky tests—too short on slow CI runners or unnecessarily long in fast environments. Consider polling for a language server capability (e.g., checking if diagnostics are available or awaiting a ready state) with a timeout.
91-91: Consider finding positions dynamically rather than hardcoding.The hardcoded
Position(8, 25)is brittle if the fixture file changes. Consider searching the document text for.Values.replicaCountto determine the position dynamically, improving test maintainability.Example approach:
const text = document.getText(); const searchText = '.Values.replicaCount'; const offset = text.indexOf(searchText); const helmPosition = document.positionAt(offset + searchText.length / 2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/test/extension.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/extension.test.ts (1)
webpack.config.js (1)
path(5-5)
🔇 Additional comments (3)
package.json (3)
81-97: Helm language metadata looks good.The new
languagescontribution block correctly defines Helm language support with appropriate file pattern matching fortemplates/**directories. The aliases and filenamePatterns align with the activation events (line 19:onLanguage:helm).
106-107: Clarify lint script changes and ESLint 9 compatibility.The AI summary mentions that the lint command was updated to include
ESLINT_USE_FLAT_CONFIG=false, but this flag is not visible in the code (line 107). Additionally, the project uses ESLint v9.0.0 (line 125), which uses flat config by default. Please verify:
- Whether the environment variable is set in the CI workflow or elsewhere
- Whether ESLint 9's flat config mode is properly supported (or if the flag should be added to preserve legacy config support)
- Whether removing lint from the pretest script (line 106) affects your CI/linting gates in the workflow
122-123: Test tooling dependencies added.The addition of
@vscode/test-cliand updated@vscode/test-electronsupport the new test infrastructure mentioned in the PR context. These changes align with the pretest script updates and the CI workflow integration.
qvalentin
force-pushed
the
test/smoke-test
branch
from
8c1b18b to
6fee9fc
Compare
qvalentin
force-pushed
the
test/smoke-test
branch
from
6fee9fc to
66125cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/executable.test.ts (1)
30-30: Use a more robust unique identifier and add cleanup.The current approach using
Math.random() * 1000provides only ~1000 possible values, which increases the risk of directory name collisions in CI environments where tests may run repeatedly. Additionally, these temporary directories are never cleaned up, which could lead to disk space accumulation over time.Consider this refactor:
+import { randomUUID } from "crypto"; + class MockExtensionContext implements vscode.ExtensionContext { globalStorageUri: vscode.Uri; constructor() { this.globalStorageUri = vscode.Uri.file( - path.join(os.tmpdir(), (Math.random() * 1000).toString(), "helm-ls-test"), + path.join(os.tmpdir(), `helm-ls-test-${randomUUID()}`), ); }Also consider adding cleanup logic in an
afterEachhook:afterEach(async function() { // Clean up test directories const testDirs = await fs.readdir(os.tmpdir()); for (const dir of testDirs) { if (dir.startsWith('helm-ls-test-')) { await fs.rm(path.join(os.tmpdir(), dir), { recursive: true, force: true }); } } });
♻️ Duplicate comments (2)
src/test/extension.test.ts (2)
85-88: Activation must wait after opening document for test independence.The test waits for activation before opening the Helm document, creating a dependency on previous tests. If run in isolation, it will timeout.
Apply this diff to fix the test order:
- // Ensure extension is activated - await waitForExtensionActivation("helm-ls.helm-ls"); - const { uri: docUri } = await openHelmDocument(workspaceFolder); + + // Wait for extension to activate automatically + await waitForExtensionActivation("helm-ls.helm-ls");
120-123: Activation must wait after opening document for test independence.The test waits for activation before opening the Helm document, creating a dependency on previous tests. If run in isolation, it will timeout.
Apply this diff to fix the test order:
- // Ensure extension is activated - await waitForExtensionActivation("helm-ls.helm-ls"); - const { uri: docUri } = await openHelmDocument(workspaceFolder); + + // Wait for extension to activate automatically + await waitForExtensionActivation("helm-ls.helm-ls");
🧹 Nitpick comments (3)
src/test/executable.test.ts (1)
87-186: Comprehensive cross-platform test coverage.The test suite effectively covers all supported platforms and architectures, validates file extensions, and tests the binary reuse functionality. The 30-second timeouts are appropriate for network operations.
For enhanced confidence, you could optionally verify the downloaded binaries are actually executable:
// After downloading, verify the binary can be executed const { stdout } = await execFile(helmExecutable, ['--version']); assert.ok(stdout.includes('helm-ls'));However, this would require importing
execFilefromchild_processand may be beyond the intended scope of these download verification tests.src/test/extension.test.ts (2)
48-49: Consider parameterizing the language server initialization delay.The hardcoded 1000ms wait might be insufficient on slower CI runners or excessive on fast machines. Consider making it configurable or increasing it for more reliable test execution.
64-64: Define extension ID as a constant.The extension ID
"helm-ls.helm-ls"is repeated multiple times (lines 64, 71, 86, 121). Consider extracting it to a constant at the top of the file.Apply this diff:
+const EXT_ID = "helm-ls.helm-ls"; + /** * Helper function to wait for extension activation with timeoutThen replace all occurrences of the string with
EXT_ID.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks).vscode-test.mjs(1 hunks)README.md(1 hunks)package.json(1 hunks)src/test/executable.test.ts(2 hunks)src/test/extension.test.ts(1 hunks)src/test/fixtures/testChart/.helmignore(1 hunks)src/test/fixtures/testChart/Chart.yaml(1 hunks)src/test/fixtures/testChart/templates/NOTES.txt(1 hunks)src/test/fixtures/testChart/templates/_helpers.tpl(1 hunks)src/test/fixtures/testChart/templates/deployment.yaml(1 hunks)src/test/fixtures/testChart/values.yaml(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/fixtures/testChart/.helmignore
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- .vscode-test.mjs
- src/test/fixtures/testChart/Chart.yaml
- src/test/fixtures/testChart/templates/NOTES.txt
- tsconfig.json
- .github/workflows/ci.yml
- package.json
- src/test/fixtures/testChart/values.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/executable.test.ts (1)
src/util/executable.ts (1)
downloadHelmLs(136-189)
src/test/extension.test.ts (1)
src/extension.ts (2)
activate(25-82)deactivate(84-90)
🪛 YAMLlint (1.37.1)
src/test/fixtures/testChart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
🔇 Additional comments (9)
src/test/fixtures/testChart/templates/deployment.yaml (4)
1-6: LGTM! Standard Helm Deployment metadata.The deployment metadata follows Helm best practices with template helpers for naming and labels.
Note: The YAMLlint syntax error at line 6 is a false positive—YAMLlint doesn't recognize Helm template syntax. The
{{-is valid whitespace control in Helm templates.
7-13: LGTM! Correct autoscaling and selector logic.The conditional replica count and selector labels follow Kubernetes best practices.
35-46: Verify containerPort configuration.The containerPort is currently set to
.Values.service.port. While this may be intentional for this test fixture, it's more conventional to use a dedicated value like.Values.containerPortor.Values.service.targetPort, as service ports and container ports can differ (e.g., service on 80, container on 8080).If the service and container ports are always the same in your test scenarios, this is fine. Otherwise, consider separating these values.
47-78: LGTM! Well-structured optional configurations.The conditional blocks for probes, resources, volumes, and scheduling constraints follow Helm best practices with proper
{{- with }}guards and consistent YAML rendering.src/test/fixtures/testChart/templates/_helpers.tpl (1)
1-62: LGTM! Standard Helm chart helpers.These helper templates follow standard Helm chart patterns (typically generated by
helm create) and implement correct logic for:
- Name truncation to 63 characters per DNS naming spec
- Flexible naming with override support
- Standard Kubernetes labels following recommended conventions
- Conditional service account name resolution
All helpers are properly referenced by the deployment template.
src/test/executable.test.ts (2)
72-72: LGTM!The
keys()method addition correctly extends the mock to match the VSCode SecretStorage API, appropriately returning an empty array wrapped in a Promise.
85-188: Good approach to gate expensive E2E tests.Wrapping the test suite in a CI check is appropriate for tests that download real binaries from GitHub. This prevents slow, network-dependent tests from running during local development.
However, consider adding a comment or README documentation explaining how developers can run these tests locally when needed:
// To run these tests locally, set CI=true: // CI=true npm test if (process.env.CI) {This helps developers understand the test's availability and how to manually verify binary download functionality before pushing changes.
src/test/extension.test.ts (2)
5-27: LGTM: Robust activation polling.The helper correctly polls for extension activation with a reasonable timeout and interval.
57-77: LGTM: Correct test flow for activation.The test properly opens the Helm document before waiting for activation, ensuring the test can run independently.
| const helmHoverContent = helmHovers[0].contents[0] as vscode.MarkdownString; | ||
| assert.ok(helmHoverContent, "Hover content should exist"); | ||
|
|
||
| // Check for expected content (replicaCount value from values.yaml) | ||
| assert.ok( | ||
| helmHoverContent.value.includes("1") || | ||
| helmHoverContent.value.includes("replicaCount"), | ||
| `Hover should show replicaCount value or reference. Got: ${helmHoverContent.value}`, | ||
| ); |
There was a problem hiding this comment.
Unsafe MarkdownString cast can throw at runtime.
Hover contents can be string, MarkdownString, or MarkedString. Directly casting to MarkdownString and accessing .value will throw if the content is a plain string or has a different structure.
Add a helper function at the top of the file to safely extract hover text:
function flattenHoverText(hovers: vscode.Hover[]): string {
const parts: string[] = [];
for (const h of hovers ?? []) {
for (const c of h.contents ?? []) {
if (typeof c === 'string') {
parts.push(c);
continue;
}
const anyC = c as any;
if (typeof anyC.value === 'string') {
parts.push(anyC.value);
continue;
}
}
}
return parts.join('\n');
}Then replace the unsafe casting:
- const helmHoverContent = helmHovers[0].contents[0] as vscode.MarkdownString;
- assert.ok(helmHoverContent, "Hover content should exist");
-
- // Check for expected content (replicaCount value from values.yaml)
- assert.ok(
- helmHoverContent.value.includes("1") ||
- helmHoverContent.value.includes("replicaCount"),
- `Hover should show replicaCount value or reference. Got: ${helmHoverContent.value}`,
- );
+ const helmText = flattenHoverText(helmHovers);
+ assert.ok(helmText.length > 0, "Hover content should exist");
+ assert.ok(
+ helmText.includes("1") || helmText.includes("replicaCount"),
+ `Hover should show replicaCount value or reference. Got: ${helmText}`,
+ );🤖 Prompt for AI Agents
In src/test/extension.test.ts around lines 103 to 111, the test unsafely casts
hover contents to MarkdownString and reads .value, which can throw when hover
content is a plain string or other type; add the suggested flattenHoverText
helper at the top of the test file to safely extract a combined hover string
from vscode.Hover[], then replace the cast-and-direct-access with a call to
flattenHoverText(helmHovers) and assert against the returned string (checking
for "1" or "replicaCount"); ensure you import/declare the helper in the test
file scope and use its result in the existing assertion.
| const yamlHoverContent = yamlHovers[0].contents[0] as vscode.MarkdownString; | ||
| assert.ok(yamlHoverContent, "YAML hover content should exist"); | ||
|
|
||
| test("Sample test", () => { | ||
| assert.strictEqual(-1, [1, 2, 3].indexOf(5)); | ||
| assert.strictEqual(-1, [1, 2, 3].indexOf(0)); | ||
| // Check for Kubernetes documentation | ||
| const content = yamlHoverContent.value.toLowerCase(); | ||
| assert.ok( | ||
| content.includes("deployment") || | ||
| content.includes("spec") || | ||
| content.includes("specification"), | ||
| `Hover should show Kubernetes schema info. Got: ${yamlHoverContent.value}`, | ||
| ); |
There was a problem hiding this comment.
Unsafe MarkdownString cast can throw at runtime.
Hover contents can be string, MarkdownString, or MarkedString. Directly casting to MarkdownString and accessing .value will throw if the content is a plain string or has a different structure.
Use the flattenHoverText helper (suggested in the Helm hover test above) to safely extract text:
- const yamlHoverContent = yamlHovers[0].contents[0] as vscode.MarkdownString;
- assert.ok(yamlHoverContent, "YAML hover content should exist");
-
- // Check for Kubernetes documentation
- const content = yamlHoverContent.value.toLowerCase();
+ const content = flattenHoverText(yamlHovers).toLowerCase();
assert.ok(
content.includes("deployment") ||
content.includes("spec") ||
content.includes("specification"),
- `Hover should show Kubernetes schema info. Got: ${yamlHoverContent.value}`,
+ `Hover should show Kubernetes schema info. Got: ${content}`,
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/extension.test.ts around lines 138 to 148, the test unsafely casts
hover content to MarkdownString and reads .value which can throw for
string/MarkedString; replace the direct cast with the existing flattenHoverText
helper used elsewhere (pass yamlHovers[0].contents) to safely extract the text,
assert the returned string is truthy, then lower-case and run the same contains
checks against that flattened text instead of yamlHoverContent.value.
TODO:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores