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.

Sign up for GitHub

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

cached children fix #15036

Merged
merged 2 commits into from
Jan 18, 2024
Merged

cached children fix #15036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a12114d
fix returning cached children
amunger Jan 18, 2024
a7220d5
regression test
amunger Jan 18, 2024
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
77 changes: 51 additions & 26 deletions src/kernels/variables/JupyterVariableProvider.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ suite('JupyterVariablesProvider', () => {
let provider: JupyterVariablesProvider;
const notebook = mock<NotebookDocument>();
const cancellationToken = new CancellationTokenSource().token;
const kernel = mock<IKernel>();

const objectVariable: IVariableDescription = {
name: 'myObject',
Expand Down Expand Up @@ -69,14 +70,12 @@ suite('JupyterVariablesProvider', () => {
variables = mock<IJupyterVariables>();
kernelProvider = mock<IKernelProvider>();
provider = new JupyterVariablesProvider(instance(variables), instance(kernelProvider));
when(kernelProvider.get(anything())).thenReturn(instance(kernel));
});

test('provideVariables without parent should yield variables', async () => {
const kernel = mock<IKernel>();

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
setVariablesForParent(undefined, [objectVariable]);
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);
setVariablesForParent(undefined, [objectVariable]);

const results = await provideVariables(undefined);

Expand All @@ -86,9 +85,6 @@ suite('JupyterVariablesProvider', () => {
});

test('provideVariables with a parent should call get children correctly', async () => {
const kernel = mock<IKernel>();

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const listVariableItems = [0, 1, 2].map(createListItem);
Expand All @@ -111,7 +107,7 @@ suite('JupyterVariablesProvider', () => {
});

test('All indexed variables should be returned when requested', async () => {
const kernel = mock<IKernel>();
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const listVariable: IVariableDescription = {
name: 'myList',
Expand All @@ -121,9 +117,6 @@ suite('JupyterVariablesProvider', () => {
propertyChain: []
};

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const firstPage = [0, 1, 2].map(createListItem);
const secondPage = [3, 4, 5].map(createListItem);
setVariablesForParent(undefined, [listVariable]);
Expand All @@ -146,7 +139,7 @@ suite('JupyterVariablesProvider', () => {
});

test('Getting less indexed items than the specified count is handled', async () => {
const kernel = mock<IKernel>();
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const listVariable: IVariableDescription = {
name: 'myList',
Expand All @@ -156,9 +149,6 @@ suite('JupyterVariablesProvider', () => {
propertyChain: []
};

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const firstPage = [0, 1, 2].map(createListItem);
const secondPage = [3, 4].map(createListItem);
setVariablesForParent(undefined, [listVariable]);
Expand All @@ -182,19 +172,16 @@ suite('JupyterVariablesProvider', () => {
});

test('Getting variables again with new execution count should get updated variables', async () => {
const kernel = mock<IKernel>();
when(kernelProvider.getKernelExecution(anything()))
.thenReturn({ executionCount: 1 } as any)
.thenReturn({ executionCount: 2 } as any);

const intVariable: IVariableDescription = {
name: 'myInt',
value: '1',
root: '',
propertyChain: []
};

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
when(kernelProvider.getKernelExecution(anything()))
.thenReturn({ executionCount: 1 } as any)
.thenReturn({ executionCount: 2 } as any);

setVariablesForParent(undefined, [intVariable], [{ ...intVariable, value: '2' }]);

const first = await provideVariables(undefined);
Expand All @@ -207,17 +194,15 @@ suite('JupyterVariablesProvider', () => {
});

test('Getting variables again with same execution count should not make another call', async () => {
const kernel = mock<IKernel>();
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const intVariable: IVariableDescription = {
name: 'myInt',
value: '1',
root: '',
propertyChain: []
};

when(kernelProvider.get(anything())).thenReturn(instance(kernel));
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

setVariablesForParent(undefined, [intVariable]);

const first = await provideVariables(undefined);
Expand All @@ -229,4 +214,44 @@ suite('JupyterVariablesProvider', () => {

verify(variables.getAllVariableDiscriptions(anything(), anything(), anything(), anything())).once();
});

test('Cache pages of indexed children correctly', async () => {
when(kernelProvider.getKernelExecution(anything())).thenReturn({ executionCount: 1 } as any);

const listVariable: IVariableDescription = {
name: 'myList',
value: '[...]',
count: 6,
root: 'myList',
propertyChain: []
};

const firstPage = [0, 1, 2].map(createListItem);
const secondPage = [3, 4, 5].map(createListItem);
setVariablesForParent(undefined, [listVariable]);
setVariablesForParent(objectContaining({ root: 'myList', propertyChain: [] }), firstPage, undefined, 0);
setVariablesForParent(
objectContaining({ root: 'myList', propertyChain: [] }),
secondPage,
undefined,
firstPage.length
);

const rootVariable = (await provideVariables(undefined))[0];
await provideVariables(rootVariable!.variable, 2);

// once for the parent and once for each of the two pages of list items
verify(variables.getAllVariableDiscriptions(anything(), anything(), anything(), anything())).thrice();

const listItemResult = await provideVariables(rootVariable!.variable, 2);

assert.equal(listItemResult.length, 6, 'full list of items should be returned');
listItemResult.forEach((item, index) => {
assert.equal(item.variable.name, index.toString());
assert.equal(item.variable.value, `value${index}`);
});

// no extra calls for getting the children again
verify(variables.getAllVariableDiscriptions(anything(), anything(), anything(), anything())).thrice();
});
});
2 changes: 1 addition & 1 deletion src/kernels/variables/JupyterVariablesProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class JupyterVariablesProvider implements NotebookVariableProvider {
const variables = await parentDescription.getChildren(start, token);
results = variables.map((variable) => this.createVariableResult(variable, kernel));
this.variableResultCache.setResults(executionCount, cacheKey, results);
} else {
} else if (!results) {
// no cached results and no way to get children, so return empty
return;
}
Expand Down