Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Scalar parameter docstring and example arguments unused #479

Merged
merged 21 commits into from
Oct 7, 2024
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
8 changes: 8 additions & 0 deletions .github/workflows/pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,11 @@ jobs:
- name: integration tests
working-directory: package
run: yarn test:integration

# https://playwright.dev/docs/ci-intro#html-report
- uses: actions/upload-artifact@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where can I see this report?

if: ${{ !cancelled() }}
with:
name: playwright-report
path: package/playwright-report/
retention-days: 30
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ Every PR should come with a test that checks it.

## Changelog

### 12.0.10

- fix: Parameter docstrings not shown

### 12.0.9

- fix: decrease bundle size.
Expand Down
4 changes: 2 additions & 2 deletions package/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@kusto/monaco-kusto",
"version": "12.0.9",
"version": "12.0.10",
"description": "CSL, KQL plugin for the Monaco Editor",
"author": {
"name": "Microsoft"
Expand All @@ -24,7 +24,7 @@
"clean": "rimraf ./release",
"start": "ts-node ./scripts/dev.ts",
"test": "jest",
"test:integration": "playwright test --trace=retain-on-failure -c tests/integration/playwright.config.ts",
"test:integration": "playwright test -c tests/integration/playwright.config.ts",
"test:integration:watch": "ts-node ./scripts/test.ts"
},
"types": "./release/esm/monaco.contribution.d.ts",
Expand Down
6 changes: 3 additions & 3 deletions package/src/languageServiceManager/kustoLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1695,7 +1695,7 @@ class KustoLanguageService implements LanguageService {
const paramSymbol: sym.ScalarSymbol = Kusto.Language.Symbols.ScalarTypes.GetSymbol(
getCslTypeNameFromClrType(param.type)
);
return new sym.ParameterSymbol(param.name, paramSymbol, null);
return new sym.ParameterSymbol(param.name, paramSymbol, param.docstring ?? null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this change, others are a bit more speculative

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not clear on the use case.
Could we add a unit or integration test to clarify it and help prevent regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Examples and docstring/description are added to tooltips on hover. I'm using them for dashboard system parameters, and and I'd assume other ones are in the system we get from kusto (otherwise, IDK why the C# api would expose it)

Could we add a unit or integration test to clarify it and help prevent regressions?

Unlike a lot of other stuff, this isn't really load bearing. If it breaks it won't block a release. I don't think it should be a priority for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a unit test would help I might add that, because it would be easy, but I don't think it would do anything useful.

An integration tests would be pretty expensive to add.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry if I wasn't clear.
From my perspective, testing is an important part of our work as engineers.
It helps ensure that what we build remains reliable and understandable for other developers.
I would suggest adding an integration test here, but if that's too difficult, a unit test would also be fine.

Copy link
Contributor

@sagivf sagivf Aug 27, 2024

Choose a reason for hiding this comment

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

@maxburs can you add some examples/screenshots of where this shows up and how?
That would help in understanding how/what test we should add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

}

private static createTabularParameterSymbol(param: s.TabularParameter): sym.ParameterSymbol {
Expand All @@ -1720,13 +1720,13 @@ class KustoLanguageService implements LanguageService {
paramSymbol,
null,
null,
null,
param.examples ? KustoLanguageService.toBridgeList(param.examples) : null,
false,
null,
1,
1,
expression,
null
param.docstring ?? null
);
}

Expand Down
2 changes: 1 addition & 1 deletion package/tests/integration/completion-items.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ test.describe('completion items', () => {
expect(options).toEqual([
'EndTime',
'StartTime',
'_time_zone',
'datetime()',
'datetime_add(part, value, datetime)',
'datetime_diff(part, datetime1, datetime2)',
Expand All @@ -66,7 +67,6 @@ test.describe('completion items', () => {
'format_datetime(date, format)',
'format_timespan(timespan, format)',
'ingestion_time()',
'make_datetime(year, month, day, [hour], [minute], [second])',
]);
});

Expand Down
32 changes: 32 additions & 0 deletions package/tests/integration/doc-comments.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { test, expect } from '@playwright/test';
import { createMonaKustoModel, MonaKustoModel, loadPageAndWait } from './testkit';

test('scalar', async ({ page }) => {
await loadPageAndWait(page);
const model = createMonaKustoModel(page);

const editor = model.editor().locator;
await editor.focus();
await editor.fill('print _time_zone');

await page.getByRole('code').getByText('_time_zone').hover();
const tooltip = await page.getByRole('tooltip').locator('.rendered-markdown').innerText();
expect(tooltip).toEqual(
`_time_zone: string\n\nIANA time zone. For example: "America/Los_Angeles", UTC, or "Europe/Stockholm"`
);
});

test('tabular', async ({ page }) => {
await loadPageAndWait(page);
const model = createMonaKustoModel(page);

const editor = model.editor().locator;
await editor.focus();
await editor.fill('print _base_query');

await page.getByRole('code').getByText('_base_query').hover();
const tooltip = await page.getByRole('tooltip').locator('.rendered-markdown').innerText();
expect(tooltip).toEqual(
`_base_query: table(StartTime)\n\nBase query\nAvailability: Inline\n\nBase query will be inlined into this query`
);
});
26 changes: 25 additions & 1 deletion package/tests/integration/env/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import debounce from 'lodash/debounce';
import kustoWorkerUrl from './kustoWorker?url';
// @ts-ignore
import editorWorker from './editorWorker?url';
import type { ScalarParameter, TabularParameter } from '../../../src/monaco.contribution';

window.MonacoEnvironment = {
getWorker(_moduleId, label) {
Expand Down Expand Up @@ -100,11 +101,34 @@ window.addEventListener('resize', () => {
editor.layout();
});

const scalarParameter: ScalarParameter = {
name: '_time_zone',
type: 'string',
docstring: 'IANA time zone. For example: "America/Los_Angeles", UTC, or "Europe/Stockholm"',
};

const tabularParameter: TabularParameter = {
columns: [
{
name: 'StartTime',
type: 'datetime',
},
],
name: '_base_query',
docstring: '# Base query\n\n## Availability: Inline\n\nBase query will be inlined into this query\n',
};

getKustoWorker().then((workerAccessor) => {
const model = editor.getModel();
if (model) {
workerAccessor(model.uri).then((worker) => {
worker.setSchemaFromShowSchema(schema, 'https://help.kusto.windows.net', 'Samples');
worker.setSchemaFromShowSchema(
schema,
'https://help.kusto.windows.net',
'Samples',
[scalarParameter],
[tabularParameter]
);
});
}
});
Expand Down
12 changes: 6 additions & 6 deletions package/tests/integration/playwright.config.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { defineConfig, devices } from '@playwright/test';
import { defineConfig } from '@playwright/test';

export default defineConfig({
outputDir: './.test-results',
preserveOutput: 'failures-only',
forbidOnly: !!process.env.CI,
retries: process.env.CI ? 2 : 0,
workers: process.env.CI ? 1 : undefined,
morgilad marked this conversation as resolved.
Show resolved Hide resolved
timeout: 5_000,
expect: { timeout: 5_000 },
reporter: 'line',
// https://playwright.dev/docs/test-reporters#github-actions-annotations
morgilad marked this conversation as resolved.
Show resolved Hide resolved
// 'github' for GitHub Actions CI to generate annotations, plus a concise 'dot'
// default 'list' when running locally
reporter: process.env.CI ? [['github'], ['html']] : undefined,
use: {
trace: 'on-first-retry',
trace: 'retain-on-failure',
defaultBrowserType: 'chromium',
},
webServer: {
Expand Down
Loading