Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions packages/code-analyzer-core/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@salesforce/code-analyzer-core",
"description": "Core Package for the Salesforce Code Analyzer",
"version": "0.38.1",
"version": "0.39.0-SNAPSHOT",
"author": "The Salesforce Code Analyzer Team",
"license": "BSD-3-Clause",
"homepage": "https://developer.salesforce.com/docs/platform/salesforce-code-analyzer/overview",
Expand Down Expand Up @@ -72,4 +72,4 @@
"!src/index.ts"
]
}
}
}
22 changes: 20 additions & 2 deletions packages/code-analyzer-core/src/code-analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,21 +326,29 @@ export class CodeAnalyzer {

const runPromises: Promise<EngineRunResults>[] = ruleSelection.getEngineNames().map(async (engineName) => {
const workingFolder: string = await this.tempFolder.makeSubfolder(runWorkingFolderName, engineName);
if (this.config.getPreserveAllWorkingDirectories()) {
this.tempFolder.markToBeKept(runWorkingFolderName, engineName);
}
const engineRunOptions: engApi.RunOptions = {
logFolder: this.config.getLogFolder(),
workingFolder: workingFolder,
workspace: engApiWorkspace
};
const errorCallback: () => void = () => {
// istanbul ignore else
if (!this.tempFolder.isKept(runWorkingFolderName, engineName)) {
this.emitLogEvent(LogLevel.Debug, getMessage('EngineWorkingFolderKept', engineName, workingFolder));
this.emitLogEvent(LogLevel.Debug, getMessage('EngineWorkingFolderKeptDueToError', engineName, workingFolder));
this.tempFolder.markToBeKept(runWorkingFolderName, engineName);
}
};
const results: EngineRunResults = await this.runEngineAndValidateResults(engineName, ruleSelection, engineRunOptions, errorCallback);
await this.tempFolder.removeIfNotKept(runWorkingFolderName, engineName);
return results;
});
if (this.config.getPreserveAllWorkingDirectories()) {
this.tempFolder.markToBeKept(runWorkingFolderName);
this.emitLogEvent(LogLevel.Debug, getMessage('AllWorkingFoldersKept', await this.tempFolder.getPath(runWorkingFolderName)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This this.tempFolder.markToBeKept(runWorkingFolderName); isn't needed. Line 330 already marks the subfolders as kept and therefore the parent folder (runWorkingFolderName) is already kept by the way the temp folder preserves parents.

I'm on the fence about needing a single debug message. We could just debug each engine folder instead to keep a log code simple with the log being below line 330. But I see the motivation here - to have a single log for all things under the run folder.

const engineRunResultsList: EngineRunResults[] = await Promise.all(runPromises);

await this.tempFolder.removeIfNotKept(runWorkingFolderName);
Expand Down Expand Up @@ -377,14 +385,19 @@ export class CodeAnalyzer {

const rulePromises: Promise<RuleImpl[]>[] = this.getEngineNames().map(async (engineName) => {
const workingFolder: string = await this.tempFolder.makeSubfolder(rulesWorkingFolderName, engineName);

if (this.config.getPreserveAllWorkingDirectories()) {
this.tempFolder.markToBeKept(rulesWorkingFolderName, engineName)
}

const describeOptions: engApi.DescribeOptions = {
workspace: engApiWorkspace,
workingFolder: workingFolder,
logFolder: this.config.getLogFolder()
};
const errorCallback: () => void = () => {
if (!this.tempFolder.isKept(rulesWorkingFolderName, engineName)) {
this.emitLogEvent(LogLevel.Debug, getMessage('EngineWorkingFolderKept', engineName, workingFolder));
this.emitLogEvent(LogLevel.Debug, getMessage('EngineWorkingFolderKeptDueToError', engineName, workingFolder));
this.tempFolder.markToBeKept(rulesWorkingFolderName, engineName);
}
};
Expand All @@ -393,6 +406,11 @@ export class CodeAnalyzer {
return rules;
});

if (this.config.getPreserveAllWorkingDirectories()) {
this.tempFolder.markToBeKept(rulesWorkingFolderName);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto - not needed.

this.emitLogEvent(LogLevel.Debug, getMessage('AllWorkingFoldersKept', await this.tempFolder.getPath(rulesWorkingFolderName)));
}

this.rulesCache.set(cacheKey, (await Promise.all(rulePromises)).flat());

await this.tempFolder.removeIfNotKept(rulesWorkingFolderName);
Expand Down
14 changes: 13 additions & 1 deletion packages/code-analyzer-core/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const FIELDS = {
LOG_FOLDER: 'log_folder',
LOG_LEVEL: 'log_level',
CUSTOM_ENGINE_PLUGIN_MODULES: 'custom_engine_plugin_modules', // Hidden
PRESERVE_ALL_WORKING_DIRECTORIES: 'preserve_all_working_directories', // Hidden
RULES: 'rules',
ENGINES: 'engines',
SEVERITY: 'severity',
Expand Down Expand Up @@ -41,6 +42,7 @@ type TopLevelConfig = {
log_level: LogLevel
rules: Record<string, RuleOverrides>
engines: Record<string, EngineOverrides>
preserve_all_working_directories: boolean // INTERNAL USE ONLY
Copy link
Contributor

Choose a reason for hiding this comment

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

For properties that are potentially customer facing, let's be consistent. We are calling it "working folders" and not directories. So I propose we call this something like "preserve_working_folders_always" instead.

custom_engine_plugin_modules: string[] // INTERNAL USE ONLY
}

Expand All @@ -51,6 +53,7 @@ export const DEFAULT_CONFIG: TopLevelConfig = {
log_level: LogLevel.Debug,
rules: {},
engines: {},
preserve_all_working_directories: false, // INTERNAL USE ONLY
custom_engine_plugin_modules: [], // INTERNAL USE ONLY
};

Expand Down Expand Up @@ -136,7 +139,7 @@ export class CodeAnalyzerConfig {
configRoot = !rawConfig.config_root ? (configRoot ?? process.cwd()) :
validateAbsoluteFolder(rawConfig.config_root, FIELDS.CONFIG_ROOT);
const configExtractor: engApi.ConfigValueExtractor = new engApi.ConfigValueExtractor(rawConfig, '', configRoot);
configExtractor.addKeysThatBypassValidation([FIELDS.CUSTOM_ENGINE_PLUGIN_MODULES]); // Because custom_engine_plugin_modules is currently hidden
configExtractor.addKeysThatBypassValidation([FIELDS.CUSTOM_ENGINE_PLUGIN_MODULES, FIELDS.PRESERVE_ALL_WORKING_DIRECTORIES]); // Hidden fields bypass validation
configExtractor.validateContainsOnlySpecifiedKeys([FIELDS.CONFIG_ROOT, FIELDS.LOG_FOLDER, FIELDS.LOG_LEVEL ,FIELDS.RULES, FIELDS.ENGINES]);
const config: TopLevelConfig = {
config_root: configRoot,
Expand All @@ -145,6 +148,7 @@ export class CodeAnalyzerConfig {
custom_engine_plugin_modules: configExtractor.extractArray(FIELDS.CUSTOM_ENGINE_PLUGIN_MODULES,
engApi.ValueValidator.validateString,
DEFAULT_CONFIG.custom_engine_plugin_modules)!,
preserve_all_working_directories: configExtractor.extractBoolean(FIELDS.PRESERVE_ALL_WORKING_DIRECTORIES, DEFAULT_CONFIG.preserve_all_working_directories)!,
rules: extractRulesValue(configExtractor),
engines: extractEnginesValue(configExtractor)
}
Expand Down Expand Up @@ -226,6 +230,14 @@ export class CodeAnalyzerConfig {
return this.config.custom_engine_plugin_modules;
}

/**
* Returns a boolean indicating whether working directories should be preserved even if their engine succeeds, as opposed
* to being kept only for failed engines.
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest:

     * Returns a boolean indicating whether working folders are preserved always.
     * If false, then the working folders for an engine are deleted unless the engine issues an error.
     * If true, then the working folders for each engine remain, even if an engine does not issues any error.

*/
public getPreserveAllWorkingDirectories(): boolean {
return this.config.preserve_all_working_directories;
}

/**
* Returns a {@link RuleOverrides} instance containing the user specified overrides for all rules associated with the specified engine
* @param engineName name of the engine
Expand Down
5 changes: 4 additions & 1 deletion packages/code-analyzer-core/src/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ const MESSAGE_CATALOG : MessageCatalog = {
EngineReturnedViolationWithCodeLocationWithEndColumnBeforeStartColumnOnSameLine:
`Engine failure. The engine '%s' returned a violation for rule '%s' that contains a code location with the endLine equal to the startLine and the endColumn %d before the startColumn %d.`,

EngineWorkingFolderKept:
AllWorkingFoldersKept:
`All temporary working folders have been kept, and are located in: %s`,

EngineWorkingFolderKeptDueToError:
`Since the engine '%s' emitted an error, the following temporary working folder will not be removed: %s`
}

Expand Down
51 changes: 49 additions & 2 deletions packages/code-analyzer-core/test/code-analyzer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,18 @@ describe("Tests for the run method of CodeAnalyzer", () => {
return codeAnalyzer;
}

beforeEach(async () => {
codeAnalyzer = createCodeAnalyzer();
async function setupCodeAnalyzerWithStubs(config: CodeAnalyzerConfig = CodeAnalyzerConfig.withDefaults()): Promise<void> {
codeAnalyzer = createCodeAnalyzer(config);
sampleRunOptions = {workspace: await codeAnalyzer.createWorkspace([__dirname])};
const stubPlugin: stubs.StubEnginePlugin = new stubs.StubEnginePlugin();
await codeAnalyzer.addEnginePlugin(stubPlugin);
stubEngine1 = stubPlugin.getCreatedEngine('stubEngine1') as stubs.StubEngine1;
stubEngine2 = stubPlugin.getCreatedEngine('stubEngine2') as stubs.StubEngine2;
selection = await codeAnalyzer.selectRules([]);
}

beforeEach(async () => {
await setupCodeAnalyzerWithStubs();
});

it("When run options contains workspace with targets, then they are passed to each engine successfully", async () => {
Expand Down Expand Up @@ -861,6 +865,49 @@ describe("Tests for the run method of CodeAnalyzer", () => {
expect(fileSystem.files).not.toContain(expectedRunWorkingFolderForStubEngine3);
});


it("When running rules, if the top-level preserve_all_working_directories flag is true, all run working folders are preserved and a log is issued", async () => {
await setupCodeAnalyzerWithStubs(CodeAnalyzerConfig.fromObject({
preserve_all_working_directories: true
}));

const logEvents: LogEvent[] = [];
codeAnalyzer.onEvent(EventType.LogEvent, (event: LogEvent) => logEvents.push(event));

await codeAnalyzer.run(selection, sampleRunOptions);

const expectedRunWorkingFolderRoot: string = path.join(os.tmpdir(),'code-analyzer-0','run-' + clock.formatToDateTimeString());
const expectedRunWorkingFolderForStubEngine1: string = path.join(expectedRunWorkingFolderRoot, 'stubEngine1');
const expectedRunWorkingFolderForStubEngine2: string = path.join(expectedRunWorkingFolderRoot, 'stubEngine2');
const expectedRunWorkingFolderForStubEngine3: string = path.join(expectedRunWorkingFolderRoot, 'stubEngine3');

// First confirm that the root folder and all 3 engines run working folders were created
const createdFolders: string[] = fileSystem.mkdirCallHistory.map(args => args.absPath.toString());
expect(createdFolders).toContain(expectedRunWorkingFolderRoot);
expect(createdFolders).toContain(expectedRunWorkingFolderForStubEngine1);
expect(createdFolders).toContain(expectedRunWorkingFolderForStubEngine2);
expect(createdFolders).toContain(expectedRunWorkingFolderForStubEngine3);

// Confirm that the root folder and all 3 engines run working folders were removed (because none of them errored during run)
const removedFolders: string[] = fileSystem.rmCallHistory.map(args => args.absPath.toString());
expect(removedFolders).not.toContain(expectedRunWorkingFolderRoot);
expect(removedFolders).not.toContain(expectedRunWorkingFolderForStubEngine1);
expect(removedFolders).not.toContain(expectedRunWorkingFolderForStubEngine2);
expect(removedFolders).not.toContain(expectedRunWorkingFolderForStubEngine3);

// Verify end result
expect(fileSystem.files).toContain(expectedRunWorkingFolderRoot);
expect(fileSystem.files).toContain(expectedRunWorkingFolderForStubEngine1);
expect(fileSystem.files).toContain(expectedRunWorkingFolderForStubEngine2);
expect(fileSystem.files).toContain(expectedRunWorkingFolderForStubEngine3);

// Verify log lines
const relevantLogMsgs: string[] = logEvents.filter(e => e.logLevel === LogLevel.Debug &&
e.message.includes('All temporary working folders have been kept')).map(e => e.message);

expect(relevantLogMsgs.filter(m => m.endsWith(expectedRunWorkingFolderRoot))).toHaveLength(1);
})

it("When running rules, if an engine issues an error, then we preserve that run working folder and issue a log pointing to it", async () => {
codeAnalyzer = createCodeAnalyzer();
const logEvents: LogEvent[] = [];
Expand Down
14 changes: 14 additions & 0 deletions packages/code-analyzer-core/test/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe("Tests for creating and accessing configuration values", () => {
expect(conf.getLogFolder()).toEqual(os.tmpdir());
expect(conf.getLogLevel()).toEqual(LogLevel.Debug);
expect(conf.getCustomEnginePluginModules()).toEqual([]);
expect(conf.getPreserveAllWorkingDirectories()).toEqual(false);
expect(conf.getRuleOverridesFor("stubEngine1")).toEqual({});
expect(conf.getEngineOverridesFor("stubEngine1")).toEqual({});
expect(conf.getRuleOverridesFor("stubEngine2")).toEqual({});
Expand Down Expand Up @@ -81,6 +82,7 @@ describe("Tests for creating and accessing configuration values", () => {
const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-02.Yml'));
expect(conf.getLogFolder()).toEqual(os.tmpdir());
expect(conf.getCustomEnginePluginModules()).toEqual(['dummy_plugin_module_path']);
expect(conf.getPreserveAllWorkingDirectories()).toEqual(true);
expect(conf.getRuleOverridesFor('stubEngine1')).toEqual({});
expect(conf.getRuleOverridesFor('stubEngine2')).toEqual({
stub2RuleC: {
Expand All @@ -103,6 +105,7 @@ describe("Tests for creating and accessing configuration values", () => {
const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromFile(path.join(TEST_DATA_DIR, 'sample-config-03.json'));
expect(conf.getLogFolder()).toEqual(path.join(TEST_DATA_DIR, 'sampleLogFolder'));
expect(conf.getCustomEnginePluginModules()).toEqual([]);
expect(conf.getPreserveAllWorkingDirectories()).toEqual(false);
expect(conf.getRuleOverridesFor('stubEngine1')).toEqual({});
expect(conf.getRuleOverridesFor('stubEngine2')).toEqual({});
expect(conf.getEngineOverridesFor('stubEngine1')).toEqual({});
Expand Down Expand Up @@ -292,6 +295,17 @@ describe("Tests for creating and accessing configuration values", () => {
getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','custom_engine_plugin_modules', 'array', 'string'));
});

it("When preserve_all_working_directories is not a boolean, then throw an error", () => {
expect(() => CodeAnalyzerConfig.fromObject({preserve_all_working_directories: 3})).toThrow(
getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','preserve_all_working_directories', 'boolean', 'number'));

expect(() => CodeAnalyzerConfig.fromObject({preserve_all_working_directories: 'abcd'})).toThrow(
getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','preserve_all_working_directories', 'boolean', 'string'));

expect(() => CodeAnalyzerConfig.fromObject({preserve_all_working_directories: 'true'})).toThrow(
getMessageFromCatalog(SHARED_MESSAGE_CATALOG, 'ConfigValueMustBeOfType','preserve_all_working_directories', 'boolean', 'string'));
})

it("When supplied config_root path is a valid absolute path, then we use it", () => {
const configRootValue: string = path.join(TEST_DATA_DIR, 'sampleWorkspace');
const conf: CodeAnalyzerConfig = CodeAnalyzerConfig.fromObject({config_root: configRootValue});
Expand Down
41 changes: 41 additions & 0 deletions packages/code-analyzer-core/test/rule-selection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,47 @@ describe('Tests for selecting rules', () => {
expect(relevantLogMsgs.filter(m => m.endsWith(expectedRulesWorkingFolderForStubEngine2))).toHaveLength(1);
});

it("When selecting rules, if preserve_all_working_directories is true, then all working folders are kept regardless of failures and the root is logged", async () => {
await setupCodeAnalyzerWithStubPlugin(CodeAnalyzerConfig.fromObject({
preserve_all_working_directories: true
}));

const logEvents: LogEvent[] = [];
codeAnalyzer.onEvent(EventType.LogEvent, (event: LogEvent) => logEvents.push(event));

await codeAnalyzer.selectRules(['all']);

const expectedRulesWorkingFolderRoot: string = path.join(os.tmpdir(),'code-analyzer-0','rules-' + clock.formatToDateTimeString());
const expectedRulesWorkingFolderForStubEngine1: string = path.join(expectedRulesWorkingFolderRoot, 'stubEngine1');
const expectedRulesWorkingFolderForStubEngine2: string = path.join(expectedRulesWorkingFolderRoot, 'stubEngine2');
const expectedRulesWorkingFolderForStubEngine3: string = path.join(expectedRulesWorkingFolderRoot, 'stubEngine3');

// First confirm that the root folder and all 3 engine rule working folders were created
const createdFolders: string[] = fileSystem.mkdirCallHistory.map(args => args.absPath.toString());
expect(createdFolders).toContain(expectedRulesWorkingFolderRoot);
expect(createdFolders).toContain(expectedRulesWorkingFolderForStubEngine1);
expect(createdFolders).toContain(expectedRulesWorkingFolderForStubEngine2);
expect(createdFolders).toContain(expectedRulesWorkingFolderForStubEngine3);

// Next confirm that neither the root folder nor the subfolders were removed.
const removedFolders: string[] = fileSystem.rmCallHistory.map(args => args.absPath.toString());
expect(removedFolders).not.toContain(expectedRulesWorkingFolderRoot);
expect(removedFolders).not.toContain(expectedRulesWorkingFolderForStubEngine1);
expect(removedFolders).not.toContain(expectedRulesWorkingFolderForStubEngine2);
expect(removedFolders).not.toContain(expectedRulesWorkingFolderForStubEngine3);

// Verify end result
expect(fileSystem.files).toContain(expectedRulesWorkingFolderRoot);
expect(fileSystem.files).toContain(expectedRulesWorkingFolderForStubEngine1);
expect(fileSystem.files).toContain(expectedRulesWorkingFolderForStubEngine2);
expect(fileSystem.files).toContain(expectedRulesWorkingFolderForStubEngine3);

// Verify log lines
const relevantLogMsgs: string[] = logEvents.filter(e => e.logLevel === LogLevel.Debug &&
e.message.includes('All temporary working folders have been kept')).map(e => e.message);
expect(relevantLogMsgs.filter(m => m.endsWith(expectedRulesWorkingFolderRoot))).toHaveLength(1);
});

it("When selecting rules, if no engine errors, then we fully remove the rules working folder", async () => {
codeAnalyzer = createCodeAnalyzer();
await codeAnalyzer.addEnginePlugin(new stubs.EmptyTagEnginePlugin());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
custom_engine_plugin_modules: ["dummy_plugin_module_path"]

preserve_all_working_directories: true
rules:
stubEngine2:
stub2RuleC:
Expand All @@ -10,4 +10,4 @@ engines:
miscSetting1: true
miscSetting2:
miscSetting2A: 3
miscSetting2B: ["hello", "world"]
miscSetting2B: ["hello", "world"]
Loading