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

Jump to bottom

fix(rosetta): non-compiling snippets not reported on subsequent extracts #3260

Merged
merged 1 commit into from
Dec 16, 2021
Merged

fix(rosetta): non-compiling snippets not reported on subsequent extracts #3260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/jsii-rosetta/lib/commands/extract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export async function extractSnippets(
translator.addTabletsToCache(...Object.values(await loadAllDefaultTablets(assemblies)));

if (translator.hasCache()) {
const { translations, remaining } = translator.readFromCache(snippets);
const { translations, remaining } = translator.readFromCache(snippets, true, options.includeCompilerDiagnostics);
logging.info(`Reused ${translations.length} translations from cache`);
snippets = remaining;
}
Expand Down
5 changes: 3 additions & 2 deletions packages/jsii-rosetta/lib/rosetta-translator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,15 @@ export class RosettaTranslator {
*
* Will remove the cached snippets from the input array.
*/
public readFromCache(snippets: TypeScriptSnippet[], addToTablet = true): ReadFromCacheResults {
public readFromCache(snippets: TypeScriptSnippet[], addToTablet = true, compiledOnly = false): ReadFromCacheResults {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be fine making this the default. Let's not add switches we're not going to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think the flag is necessary here. We pass in options.includeCompilerDiagnostics into compiledOnly. Without it, rosetta:extract --no-compile will not take advantage of the cache. For example:

run rosetta:extract --no-compile. examples added to tablet and cache.
run rosetta:extract --no-compile again. we have cached examples, but they are not used because didCompile is undefined. So we get cache misses for all snippets.

Now I don't know why one would do this. But as long as we support --no-compile it feels iffy to say "btw, you won't get to use the cache anymore, haha".

Unless you simply mean you want to default to compiledOnly = true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Holy shit it breaks my brain 😅.

I think you are right though, running through some cases and the alternatives feel suboptimal. Shipping this!

const remaining = [...snippets];
const translations = new Array<TranslatedSnippet>();

let i = 0;
while (i < remaining.length) {
const fromCache = tryReadFromCache(remaining[i], this.cache, this.fingerprinter);
if (fromCache) {
// If compiledOnly is set, do not consider cached snippets that do not compile
if (fromCache && (!compiledOnly || fromCache.snippet.didCompile)) {
if (addToTablet) {
this.tablet.addSnippet(fromCache);
}
Expand Down
69 changes: 69 additions & 0 deletions packages/jsii-rosetta/test/commands/extract.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,75 @@ describe('with cache file', () => {
});
});

describe('non-compiling cached examples', () => {
let otherAssembly: TestJsiiModule;
let cacheToFile: string;
beforeEach(async () => {
// Create an assembly in a temp directory
otherAssembly = await TestJsiiModule.fromSource(
{
'index.ts': `
export class ClassA {
/**
* Some method
* @example x
*/
public someMethod() {
}
}
`,
},
{
name: 'my_assembly',
jsii: DUMMY_JSII_CONFIG,
},
);

// add non-compiling snippet to cache
cacheToFile = path.join(otherAssembly.moduleDirectory, 'test.tabl.json');
await extract.extractSnippets([otherAssembly.moduleDirectory], {
cacheToFile,
includeCompilerDiagnostics: true,
validateAssemblies: false,
});

const tablet = await LanguageTablet.fromFile(cacheToFile);
expect(tablet.count).toEqual(1);
const tr = tablet.tryGetSnippet(tablet.snippetKeys[0]);
expect(tr?.snippet.didCompile).toBeFalsy();
});

afterEach(async () => assembly.cleanup());

test('are ignored with strict mode', async () => {
// second run of extract snippets should still evaluate the snippet
// even though it is present in the cache
const translationFunction = jest.fn().mockResolvedValue({ diagnostics: [], translatedSnippets: [] });
await extract.extractSnippets([otherAssembly.moduleDirectory], {
cacheToFile,
cacheFromFile: cacheToFile,
includeCompilerDiagnostics: true,
validateAssemblies: false,
translatorFactory: (o) => new MockTranslator(o, translationFunction),
});

expect(translationFunction).toHaveBeenCalledTimes(1);
});

test('are utilized with strict mode off', async () => {
const translationFunction = jest.fn().mockResolvedValue({ diagnostics: [], translatedSnippets: [] });
await extract.extractSnippets([otherAssembly.moduleDirectory], {
cacheToFile,
cacheFromFile: cacheToFile,
includeCompilerDiagnostics: false,
validateAssemblies: false,
translatorFactory: (o) => new MockTranslator(o, translationFunction),
});

expect(translationFunction).toHaveBeenCalledTimes(0);
});
});

test('do not ignore example strings', async () => {
// Create an assembly in a temp directory
const otherAssembly = await TestJsiiModule.fromSource(
Expand Down