Skip to content

NEW: @W-18173799@: Update config command to not quit if engine valida… #1773

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

Merged
stephen-carter-at-sf merged 1 commit into from
Apr 1, 2025
Merged

NEW: @W-18173799@: Update config command to not quit if engine valida… #1773

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
8 changes: 7 additions & 1 deletion src/lib/models/ConfigModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,13 @@ abstract class YamlFormatter {

private toYamlEngineOverridesForEngine(engineName: string): string {
const engineConfigDescriptor: ConfigDescription = this.codeAnalyzer.getEngineConfigDescription(engineName);
const userEngineConfig: EngineConfig = this.codeAnalyzer.getEngineConfig(engineName);

// We get the normalized (validated) engine config if it is available. But if an engine failed validation then
// getEngineConfig will just error... so instead we fall back to the raw config that the user may have supplied
// to keep things untouched. This is ok because we are already throwing an error log event displaying the issue.
// So now users can still see create a config file and make changes as needed.
const userEngineConfig: EngineConfig = this.codeAnalyzer.getEngineNames().includes(engineName) ?
this.codeAnalyzer.getEngineConfig(engineName) : this.codeAnalyzer.getConfig().getEngineOverridesFor(engineName);

let yamlCode: string = '\n' +
this.toYamlSectionHeadingComment(engineConfigDescriptor.overview) + '\n' +
Expand Down
62 changes: 50 additions & 12 deletions test/lib/actions/ConfigAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {ConfigActionSummaryViewer} from '../../../src/lib/viewers/ActionSummaryV

import {SpyConfigWriter} from '../../stubs/SpyConfigWriter';
import {SpyConfigViewer} from '../../stubs/SpyConfigViewer';
import {DisplayEventType, SpyDisplay} from '../../stubs/SpyDisplay';
import {DisplayEvent, DisplayEventType, SpyDisplay} from '../../stubs/SpyDisplay';
import { LogEventDisplayer } from '../../../src/lib/listeners/LogEventListener';

const PATH_TO_FIXTURES = path.join(__dirname, '..', '..', 'fixtures');

Expand All @@ -32,7 +33,7 @@ describe('ConfigAction tests', () => {
beforeEach(() => {
spyDisplay = new SpyDisplay();
dependencies = {
logEventListeners: [],
logEventListeners: [new LogEventDisplayer(spyDisplay)],
progressEventListeners: [],
viewer: new ConfigStyledYamlViewer(spyDisplay),
configFactory: new StubCodeAnalyzerConfigFactory(),
Expand Down Expand Up @@ -158,7 +159,7 @@ describe('ConfigAction tests', () => {
stubConfigFactory = new AlternativeStubCodeAnalyzerConfigFactory();
spyDisplay = new SpyDisplay();
dependencies = {
logEventListeners: [],
logEventListeners: [new LogEventDisplayer(spyDisplay)],
Copy link
Contributor Author

@stephen-carter-at-sf stephen-carter-at-sf Apr 1, 2025

Choose a reason for hiding this comment

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

Took me forever to understand why this test wasn't producing the error messages in the display. Then I realized we do not have the log event listeners or progress listeners added. I only added in the log event listeners here - wasn't sure if the progress listeners would cause other tests to fail or not.

progressEventListeners: [],
viewer: new ConfigStyledYamlViewer(spyDisplay),
configFactory: stubConfigFactory,
Expand Down Expand Up @@ -332,25 +333,44 @@ describe('ConfigAction tests', () => {
expect(output).toContain('disable_engine: true # Modified from: false');
});

it('Edge Case: When plugin throws error when attempting to create engine config and engine is enabled, then error', async () => {
it('Edge Case: When plugin throws error when attempting to create engine config but engine is disabled, then do not error', async () => {
dependencies.pluginsFactory = new FactoryForThrowingEnginePlugin();
dependencies.configFactory = new StubCodeAnalyzerConfigFactory();
dependencies.configFactory = new StubCodeAnalyzerConfigFactory(CodeAnalyzerConfig.fromObject({
engines: {
uncreatableEngine: {
disable_engine: true,
someField: 'some non default value'
}
}
}));

await expect(runActionAndGetDisplayedConfig(dependencies, ['NoRuleHasThisTag'])).rejects.toThrowError();
const output: string = await runActionAndGetDisplayedConfig(dependencies, ['NoRuleHasThisTag']);

expect(spyDisplay.getDisplayEvents().filter(e => e.type == DisplayEventType.ERROR)).toHaveLength(0);

expect(output).toContain('disable_engine: true # Modified from: false');
expect(output).toContain('someField: some non default value # Modified from: "someDefault"');
});

it('Edge Case: When plugin thrown error when attempting to create engine config but engine is disabled, then do not error', async () => {
it('Edge Case: When plugin throws error when attempting to create engine config, but engine is enabled, then issue error log but continue with whatever is in the users config for that engine', async () => {
dependencies.pluginsFactory = new FactoryForThrowingEnginePlugin();
dependencies.configFactory = new StubCodeAnalyzerConfigFactory(CodeAnalyzerConfig.fromObject({
engines: {
uncreatableEngine: {
disable_engine: true
disable_engine: false,
someField: 'some non default value'
}
}
}));

const output: string = await runActionAndGetDisplayedConfig(dependencies, ['NoRuleHasThisTag']);
expect(output).toContain('disable_engine: true # Modified from: false');
const output: string = await runActionAndGetDisplayedConfig(dependencies, ['uncreatableEngine']);

const errorEvents: DisplayEvent[] = spyDisplay.getDisplayEvents().filter(e => e.type == DisplayEventType.ERROR);
expect(errorEvents).toHaveLength(1);
expect(errorEvents[0].data).toContain('Error thrown by createEngineConfig');

expect(output).toContain('disable_engine: false');
expect(output).toContain('someField: some non default value # Modified from: "someDefault"');
});

it.each([
Expand Down Expand Up @@ -515,8 +535,13 @@ describe('ConfigAction tests', () => {

// ==== OUTPUT PROCESSING ====
const displayEvents = spyDisplay.getDisplayEvents();
expect(displayEvents[4].type).toEqual(DisplayEventType.LOG);
return ansis.strip(displayEvents[4].data);
if (displayEvents[4].type === DisplayEventType.LOG) {
return ansis.strip(displayEvents[4].data);
} else if (displayEvents[5].type === DisplayEventType.LOG) {
return ansis.strip(displayEvents[5].data);
} else {
return 'Could Not Get Specific Output';
}
Comment on lines +538 to +544
Copy link
Contributor Author

@stephen-carter-at-sf stephen-carter-at-sf Apr 1, 2025

Choose a reason for hiding this comment

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

I'm not a fan of how this test file assumes [4] is the output. It makes the test fragile. At some point we should improve this - but for now I'm just adding in some conditional code to account for the fact that there might be an error display event before the output display event.

}
});

Expand Down Expand Up @@ -806,6 +831,19 @@ class ThrowingEnginePlugin extends EngineApi.EnginePluginV1 {
return ['uncreatableEngine'];
}

describeEngineConfig(): EngineApi.ConfigDescription {
return {
overview: 'Some Overview',
fieldDescriptions: {
someField: {
descriptionText: 'some description',
valueType: 'string',
defaultValue: 'someDefault'
}
}
}
}

createEngineConfig(_engineName: string, _configValueExtractor: EngineApi.ConfigValueExtractor): Promise<EngineApi.ConfigObject> {
throw new Error('Error thrown by createEngineConfig');
}
Expand Down