Skip to content

Commit

Permalink
fix(rosetta): infused snippets not returned from cache (#3291)
Browse files Browse the repository at this point in the history
Infused snippets do not have the right full source since we are adding the local fixture to the snippet
rather than the fixture from where the snippet came from. Since there is no hope in actually translating
an infused snippet (unless it is in the same directory), we might as well take any cache result as is.
So this PR adds logic that returns the cache result for infused snippets always.

This was tested on my local machine and dropped all errors from infused snippets. There are still errors
from directories that are not yet strict, which is understandable.

Also, the infuse command was missing `cache-from` argument that is present in `infuseOptions`.
We were using those options as part of `rosetta extract --infuse` but should be available via `rosetta infuse`
as well.

Also fixed up minor mistake in tests where I was testing for `infused=true` instead of `infused=''`.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
kaizencc authored Dec 24, 2021
1 parent e7f1f1a commit dd44431
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 33 deletions.
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
// 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

0 comments on commit dd44431

Please sign in to comment.