Skip to content

Commit

Permalink
fix(rosetta): non-compiling snippets not reported on subsequent extra…
Browse files Browse the repository at this point in the history
…cts (#3260)

Picture this scenario: there is a non-compiling snippet in a README.md file.
I run `yarn rosetta:extract --compile` and it returns diagnostics as expected.
Behind the scenes, `.jsii.tabl.json` gets created as a cache, and this snippet
is inserted into that file with `didCompile = false`.

_Without making any changes_, I run `yarn rosetta:extract --compile` again.
This time, no diagnostics are returned, and it looks like my errors have
magically fixed themselves. However what is really happening is that `extract`
is finding a snippet in the cache that matches the offending snippet (since
I changed nothing). It is then filtering out that cached snippet, meaning we
do not actually try to compile the snippet again. This is bad; `extract` should
honor the `--compile` flag and return errors the second time around too.

There are two ways to solve this (that I can think of): we can return
diagnostics for cached snippets as well, or we can ignore non-compiling
cached snippets when `--compile` is set. I have opted for the second solution
for this reason: it is possible that I am intending to rerun the same snippet
with the expectation that I have changed _something else_ that will result
in a successful compilation (for example, I add an import to the fixture).
Open to dialogue about this.

---

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 16, 2021
1 parent 203b531 commit 771190b
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 3 deletions.
2 changes: 1 addition & 1 deletion packages/jsii-rosetta/lib/commands/extract.ts
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
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 {
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
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

0 comments on commit 771190b

Please sign in to comment.