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(rosetta): infused snippets not returned from cache #3291

Merged
merged 4 commits into from
Dec 24, 2021
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
26 changes: 23 additions & 3 deletions packages/jsii-rosetta/bin/jsii-rosetta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,19 +82,39 @@ function main() {
describe: 'Output file to store logging results. Ignored if -log is not true',
default: DEFAULT_INFUSION_RESULTS_NAME,
})
.option('cache-from', {
alias: 'C',
type: 'string',
// eslint-disable-next-line prettier/prettier
describe:
'Reuse translations from the given tablet file if the snippet and type definitions did not change',
requiresArg: true,
default: undefined,
})
.option('cache-to', {
alias: 'o',
type: 'string',
describe: 'Append all translated snippets to the given tablet file',
requiresArg: true,
default: undefined,
}),
})
.option('cache', {
alias: 'k',
type: 'string',
describe: 'Alias for --cache-from and --cache-to together',
requiresArg: true,
default: undefined,
})
.conflicts('cache', 'cache-from')
.conflicts('cache', 'cache-to'),
wrapHandler(async (args) => {
const absAssemblies = (args.ASSEMBLY.length > 0 ? args.ASSEMBLY : ['.']).map((x) => path.resolve(x));
const cacheToFile = fmap(args['cache-to'], path.resolve);
const absCacheFrom = fmap(args.cache ?? args['cache-from'], path.resolve);
const absCacheTo = fmap(args.cache ?? args['cache-to'], path.resolve);
const result = await infuse(absAssemblies, {
logFile: args['log-file'],
cacheToFile: cacheToFile,
cacheToFile: absCacheTo,
cacheFromFile: absCacheFrom,
});

let totalTypes = 0;
Expand Down
11 changes: 11 additions & 0 deletions packages/jsii-rosetta/lib/rosetta-translator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,13 @@ export class RosettaTranslator {
function tryReadFromCache(sourceSnippet: TypeScriptSnippet, cache: LanguageTablet, fingerprinter: TypeFingerprinter) {
const fromCache = cache.tryGetSnippet(snippetKey(sourceSnippet));

// infused snippets won't pass the full source check or the fingerprinter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do they not pass the fingerprinter? I tried to investigate this, and I can't really make sense of it.
Verified that the fqns are the same between fromCache.fqnsReferenced() and fromCache.snippet.fqnsReferenced. The only difference is that locally I removed the monocdk assembly and the fingerprinter takes in all assemblies at initialization. But I fail to see how that impacts anything here.

A bit of a cop out, but I think its unnecessary. We cant translate infused examples, so we might as well take what we've got. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised as well, but I agree with the solution. It's fine like this.

// but there is no reason to try to recompile it, so return cached snippet
// if there exists one.
if (isInfused(sourceSnippet)) {
return fromCache;
}

const cacheable =
fromCache &&
completeSource(sourceSnippet) === fromCache.snippet.fullSource &&
Expand All @@ -160,6 +167,10 @@ function tryReadFromCache(sourceSnippet: TypeScriptSnippet, cache: LanguageTable
return cacheable ? fromCache : undefined;
}

function isInfused(snippet: TypeScriptSnippet) {
return snippet.parameters?.infused !== undefined;
}

export interface ReadFromCacheResults {
readonly translations: TranslatedSnippet[];
readonly remaining: TypeScriptSnippet[];
Expand Down
97 changes: 67 additions & 30 deletions packages/jsii-rosetta/test/commands/extract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -432,44 +432,83 @@ test('extract and infuse in one command', async () => {
expect(types!['my_assembly.ClassA'].docs?.example).toBeDefined();
});

test('infused examples skip loose mode', async () => {
const otherAssembly = await TestJsiiModule.fromSource(
{
'index.ts': `
/**
* ClassA
*
* @exampleMetadata lit=integ.test.ts
* @example x
*/
export class ClassA {
public someMethod() {
describe('infused examples', () => {
let infusedAssembly: TestJsiiModule;
beforeEach(async () => {
infusedAssembly = await TestJsiiModule.fromSource(
{
'index.ts': `
/**
* ClassA
*
* @exampleMetadata infused
* @example x
*/
export class ClassA {
public someMethod() {
}
}
}
`,
},
{
name: 'my_assembly',
jsii: DUMMY_JSII_CONFIG,
},
);
try {
const cacheToFile = path.join(otherAssembly.moduleDirectory, 'test.tabl.json');
`,
},
{
name: 'my_assembly',
jsii: DUMMY_JSII_CONFIG,
},
);
});

afterEach(async () => {
await infusedAssembly.cleanup();
});

test('always returned from cache', async () => {
const cacheFile = path.join(infusedAssembly.moduleDirectory, 'test.tabl.json');

// Cache to file
await extract.extractSnippets([infusedAssembly.moduleDirectory], {
cacheToFile: cacheFile,
...defaultExtractOptions,
});

// Update the example with a fixture that would fail compilation
// Nothing like this should happen in practice
infusedAssembly.assembly.types!['my_assembly.ClassA'].docs!.custom!.exampleMetadata =
'infused fixture=myfix.ts-fixture';
await infusedAssembly.updateAssembly();

// Expect to return cached snippet regardless of change
// No compilation should happen
const translationFunction = jest.fn().mockResolvedValue({ diagnostics: [], translatedSnippets: [] });
await extract.extractSnippets([infusedAssembly.moduleDirectory], {
cacheFromFile: cacheFile,
...defaultExtractOptions,
translatorFactory: (o) => new MockTranslator(o, translationFunction),
});

expect(translationFunction).not.toHaveBeenCalled();
});

test('skip loose mode', async () => {
// Remove infused for now and add lit metadata that should fail
infusedAssembly.assembly.types!['my_assembly.ClassA'].docs!.custom!.exampleMetadata = 'lit=integ.test.ts';
await infusedAssembly.updateAssembly();

const cacheToFile = path.join(infusedAssembly.moduleDirectory, 'test.tabl.json');

// Without exampleMetadata infused, expect an error
await expect(
extract.extractSnippets([otherAssembly.moduleDirectory], {
extract.extractSnippets([infusedAssembly.moduleDirectory], {
cacheToFile,
...defaultExtractOptions,
}),
).rejects.toThrowError(/Sample uses literate source/);

// Add infused=true to metadata and update assembly
otherAssembly.assembly.types!['my_assembly.ClassA'].docs!.custom!.exampleMetadata = 'lit=integ.test.ts infused';
await otherAssembly.updateAssembly();
// Add infused to metadata and update assembly
infusedAssembly.assembly.types!['my_assembly.ClassA'].docs!.custom!.exampleMetadata = 'lit=integ.test.ts infused';
await infusedAssembly.updateAssembly();

// Expect same function call to succeed now
await extract.extractSnippets([otherAssembly.moduleDirectory], {
await extract.extractSnippets([infusedAssembly.moduleDirectory], {
cacheToFile,
...defaultExtractOptions,
});
Expand All @@ -478,9 +517,7 @@ test('infused examples skip loose mode', async () => {
expect(tablet.count).toEqual(1);
const tr = tablet.tryGetSnippet(tablet.snippetKeys[0]);
expect(tr?.originalSource.source).toEqual('x');
} finally {
await otherAssembly.cleanup();
}
});
});

test('infused examples have no diagnostics', async () => {
Expand Down