-
Notifications
You must be signed in to change notification settings - Fork 54
@W-14980290@: Implement Pmd7CommandInfo to allow us to call pmd7 jars #1352
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,10 +216,10 @@ export class CpdEngine extends AbstractRuleEngine { | |
| for (const occ of occurences) { | ||
| // create a violation for each occurence of the code fragment | ||
| const violation: RuleViolation = { | ||
| line: occ.attributes.line as number, | ||
| column: occ.attributes.column as number, | ||
| endLine: occ.attributes.endline as number, | ||
| endColumn: occ.attributes.endcolumn as number, | ||
| line: Number(occ.attributes.line), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out that "as number" doesn't actually change the type here from string to number. So using "Number(...)" instead so that I can actually test it as a number. |
||
| column: Number(occ.attributes.column), | ||
| endLine: Number(occ.attributes.endline), | ||
| endColumn: Number(occ.attributes.endcolumn), | ||
| ruleName: CpdRuleName, | ||
| severity: CpdViolationSeverity, | ||
| message: getMessage(BundleName.CpdEngine, "CpdViolationMessage", [codeFragmentID, occCount, occurences.length, duplication.attributes.lines, duplication.attributes.tokens]), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| import {PMD6_LIB, PMD6_VERSION} from "../../Constants"; | ||
| import {PMD6_LIB, PMD6_VERSION, PMD7_LIB, PMD7_VERSION} from "../../Constants"; | ||
| import * as path from 'path'; | ||
|
|
||
| const PMD6_MAIN_CLASS = 'net.sourceforge.pmd.PMD'; | ||
| const CPD6_MAIN_CLASS = 'net.sourceforge.pmd.cpd.CPD'; | ||
| const PMD7_CLI_CLASS = 'net.sourceforge.pmd.cli.PmdCli'; | ||
| const HEAP_SIZE = '-Xmx1024m'; | ||
|
|
||
| export interface PmdCommandInfo { | ||
|
|
@@ -40,6 +41,33 @@ export class Pmd6CommandInfo implements PmdCommandInfo { | |
| constructJavaCommandArgsForCpd(fileList: string, minimumTokens: number, language: string): string[] { | ||
| const classpath = `${PMD6_LIB}/*`; | ||
| return ['-cp', classpath, HEAP_SIZE, CPD6_MAIN_CLASS, '--filelist', fileList, | ||
| '--format', 'xml', '--minimum-tokens', String(minimumTokens), '--language', language]; | ||
| '--format', 'xml', '--minimum-tokens', minimumTokens.toString(), '--language', language]; | ||
| } | ||
| } | ||
|
|
||
| export class Pmd7CommandInfo implements PmdCommandInfo { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a lot of code here at all. But I had to add quite a bit of testing (because of the poor health of our test code). All this new code is covered 100%. |
||
| getVersion(): string { | ||
| return PMD7_VERSION; | ||
| } | ||
|
|
||
| getJarPathForLanguage(language: string): string { | ||
| return path.join(PMD7_LIB, `pmd-${language}-${this.getVersion()}.jar`); | ||
| } | ||
|
|
||
| constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string): string[] { | ||
| const classpath = classPathsForExternalRules.concat([`${PMD7_LIB}/*`]).join(path.delimiter); | ||
| const args = ['-cp', classpath, HEAP_SIZE, PMD7_CLI_CLASS, 'check', '--file-list', fileList, | ||
| '--format', 'xml']; | ||
| if (rulesets.length > 0) { | ||
| args.push('--rulesets', rulesets); | ||
| } | ||
| return args; | ||
| } | ||
|
|
||
| constructJavaCommandArgsForCpd(fileList: string, minimumTokens: number, language: string): string[] { | ||
| const classpath = `${PMD7_LIB}/*`; | ||
| const resolvedLanguage = language === 'visualforce' ? 'vf' : language; | ||
| return ['-cp', classpath, HEAP_SIZE, PMD7_CLI_CLASS, 'cpd', '--file-list', fileList, '--format', 'xml', | ||
| '--minimum-tokens', minimumTokens.toString(), '--language', resolvedLanguage, '--skip-lexical-errors']; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ export type Rule = { | |
| // be OR'd together in this property. | ||
| defaultConfig?: ESRuleConfigValue; | ||
| url?: string; | ||
| message?: string; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In RuleDescribeAction, we referenced the 'message' field dynamically... but now i'm referencing it statically. This wasn't possible because we were missing the message field from our type here. So I fixed this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you help me understand what a "message" would be in a Rule object? "description" would be the description of the rule and the Violation type should have information that the engine returns, so I'm not sure what a "message" would be here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example (even before my changes): |
||
| } | ||
|
|
||
| export type TelemetryData = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import {FakeDisplay} from "../FakeDisplay"; | ||
| import {initContainer} from "../../../src/ioc.config"; | ||
| import {RuleFilterFactoryImpl} from "../../../src/lib/RuleFilterFactory"; | ||
| import {Pmd7CommandInfo, PmdCommandInfo} from "../../../src/lib/pmd/PmdCommandInfo"; | ||
| import {Controller} from "../../../src/Controller"; | ||
| import {after} from "mocha"; | ||
| import {Inputs} from "../../../src/types"; | ||
| import {expect} from "chai"; | ||
| import {RuleDescribeAction} from "../../../src/lib/actions/RuleDescribeAction"; | ||
| import {AnyJson} from "@salesforce/ts-types"; | ||
|
|
||
| describe("Tests for RuleDescribeAction", () => { | ||
| let display: FakeDisplay; | ||
| let ruleDescribeAction: RuleDescribeAction; | ||
| before(() => { | ||
| initContainer(); | ||
| }); | ||
| beforeEach(() => { | ||
| display = new FakeDisplay(); | ||
| ruleDescribeAction = new RuleDescribeAction(display, new RuleFilterFactoryImpl()); | ||
| }); | ||
|
|
||
| describe("Tests to confirm that PMD7 binary files are invoked when choosing PMD7 with pmd engine", () => { | ||
|
|
||
| // TODO: Soon we will have an input flag to control this. Once we do, we can update this to use that instead | ||
| const originalPmdCommandInfo: PmdCommandInfo = Controller.getActivePmdCommandInfo() | ||
| before(() => { | ||
| Controller.setActivePmdCommandInfo(new Pmd7CommandInfo()); | ||
| }); | ||
| after(() => { | ||
| Controller.setActivePmdCommandInfo(originalPmdCommandInfo); | ||
| }); | ||
|
|
||
| it("When using PMD7, the rule description for a pmd rule should give correct info from PMD 7", async () => { | ||
| const inputs: Inputs = { | ||
| rulename: 'ApexCRUDViolation' | ||
| } | ||
| await ruleDescribeAction.run(inputs); | ||
|
|
||
| const rule: AnyJson = display.getLastStyledObject(); | ||
| expect(rule['name']).to.equal('ApexCRUDViolation'); | ||
| expect(rule['engine']).to.equal('pmd'); | ||
| expect(rule['isPilot']).to.equal(false); | ||
| expect(rule['enabled']).to.equal(true); | ||
| expect(rule['categories']).to.deep.equal(['Security']); | ||
| expect(rule['rulesets']).to.contain('quickstart'); | ||
| expect(rule['languages']).to.deep.equal(['apex']); | ||
| expect(rule['description']).to.have.length.greaterThan(0); | ||
| expect(rule['message']).to.have.length.greaterThan(0); | ||
| }) | ||
|
|
||
| it("When using PMD7, the rule description for a cpd rule should give back correct info from PMD 7", async () => { | ||
| const inputs: Inputs = { | ||
| rulename: 'copy-paste-detected' | ||
| } | ||
| await ruleDescribeAction.run(inputs); | ||
|
|
||
| const rule: AnyJson = display.getLastStyledObject(); | ||
| expect(rule['name']).to.equal('copy-paste-detected'); | ||
| expect(rule['engine']).to.equal('cpd'); | ||
| expect(rule['isPilot']).to.equal(false); | ||
| expect(rule['enabled']).to.equal(false); | ||
| expect(rule['categories']).to.deep.equal(['Copy/Paste Detected']); | ||
| expect(rule['rulesets']).to.deep.equal([]); | ||
| expect(rule['languages']).to.deep.equal(['apex', 'java', 'visualforce', 'xml']); | ||
| expect(rule['description']).to.have.length.greaterThan(0); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import {FakeDisplay} from "../FakeDisplay"; | ||
| import {initContainer} from "../../../src/ioc.config"; | ||
| import {RuleFilterFactoryImpl} from "../../../src/lib/RuleFilterFactory"; | ||
| import {RuleListAction} from "../../../src/lib/actions/RuleListAction"; | ||
| import {Pmd7CommandInfo, PmdCommandInfo} from "../../../src/lib/pmd/PmdCommandInfo"; | ||
| import {Controller} from "../../../src/Controller"; | ||
| import {after} from "mocha"; | ||
| import {Inputs} from "../../../src/types"; | ||
| import {expect} from "chai"; | ||
| import {Ux} from "@salesforce/sf-plugins-core"; | ||
| import {PMD7_LIB} from "../../../src/Constants"; | ||
|
|
||
| describe("Tests for RuleListAction", () => { | ||
| let display: FakeDisplay; | ||
| let ruleListAction: RuleListAction; | ||
| before(() => { | ||
| initContainer(); | ||
| }); | ||
| beforeEach(() => { | ||
| display = new FakeDisplay(); | ||
| ruleListAction = new RuleListAction(display, new RuleFilterFactoryImpl()); | ||
| }); | ||
|
|
||
| describe("Tests to confirm that PMD7 binary files are invoked when choosing PMD7", () => { | ||
|
|
||
| // TODO: Soon we will have an input flag to control this. Once we do, we can update this to use that instead | ||
| const originalPmdCommandInfo: PmdCommandInfo = Controller.getActivePmdCommandInfo() | ||
| before(() => { | ||
| Controller.setActivePmdCommandInfo(new Pmd7CommandInfo()); | ||
| }); | ||
| after(() => { | ||
| Controller.setActivePmdCommandInfo(originalPmdCommandInfo); | ||
| }); | ||
|
|
||
| it("When using PMD7, the rule list for the pmd engine should give back rules for PMD 7", async () => { | ||
| const inputs: Inputs = { | ||
| engine: ['pmd'] | ||
| } | ||
| await ruleListAction.run(inputs); | ||
|
|
||
| let tableData: Ux.Table.Data[] = display.getLastTableData(); | ||
| expect(tableData).to.have.length(67); | ||
| for (const rowData of tableData) { | ||
| expect(rowData.engine).to.equal("pmd"); | ||
| expect(rowData.sourcepackage).to.contain(PMD7_LIB); | ||
| expect(rowData.name).to.have.length.greaterThan(0); | ||
| expect(rowData.categories).to.have.length.greaterThan(0); | ||
| expect(rowData.isDfa).to.equal(false); | ||
| expect(rowData.isPilot).to.equal(false); | ||
| expect(rowData.languages).to.have.length.greaterThan(0); | ||
| } | ||
| }) | ||
|
|
||
| it("When using PMD7, the rule list for the cpd engine should give back the copy-paste-detected rule", async () => { | ||
| const inputs: Inputs = { | ||
| engine: ['cpd'] | ||
| } | ||
| await ruleListAction.run(inputs); | ||
|
|
||
| let tableData: Ux.Table.Data[] = display.getLastTableData(); | ||
| expect(tableData).to.have.length(1); | ||
| expect(tableData[0].engine).to.equal("cpd"); | ||
| expect(tableData[0].sourcepackage).to.equal("cpd"); | ||
| expect(tableData[0].name).to.equal("copy-paste-detected"); | ||
| expect(tableData[0].categories).to.deep.equal(["Copy/Paste Detected"]); | ||
| expect(tableData[0].rulesets).to.deep.equal([]); | ||
| expect(tableData[0].isDfa).to.equal(false); | ||
| expect(tableData[0].isPilot).to.equal(false); | ||
| expect(tableData[0].languages).to.deep.equal(['apex', 'java', 'visualforce', 'xml']); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling this TODO right now because the testability is impossible without having this data tracked on our own Display object.
I confirmed that this did not break rule describe or rule describe --json in any way. Still outputting the same information and all of our other tests around this still pass.