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 (PMD): @W-15708421@: Read PMD results from outfile instead of stdout. #1551

Merged
merged 2 commits into from
Jul 24, 2024
Merged

FIX (PMD): @W-15708421@: Read PMD results from outfile instead of stdout. #1551

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a90c1e0
FIX (PMD): @W-15708421@: Read PMD results from outfile instead of std…
jfeingold35 Jul 24, 2024
2234ecf
PMD writes stdout to logfile.
jfeingold35 Jul 24, 2024
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
6 changes: 3 additions & 3 deletions src/lib/pmd/PmdCommandInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export interface PmdCommandInfo {

getJarPathForLanguage(lang: string): string

constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string): string[]
constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string, outfile: string): string[]

constructJavaCommandArgsForCpd(fileList: string, minimumTokens: number, language: string): string[]
}
Expand All @@ -22,10 +22,10 @@ export class Pmd7CommandInfo implements PmdCommandInfo {
return path.join(PMD7_LIB, `pmd-${language}-${this.getVersion()}.jar`);
}

constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string): string[] {
constructJavaCommandArgsForPmd(fileList: string, classPathsForExternalRules: string[], rulesets: string, outfile: string): string[] {
const classpath = classPathsForExternalRules.concat([`${PMD7_LIB}/*`]).join(path.delimiter);
const args = ['-cp', classpath, PMD7_CLI_CLASS, 'check', '--file-list', fileList,
'--format', 'xml'];
'--format', 'xml', '-r', outfile];
if (rulesets.length > 0) {
args.push('--rulesets', rulesets);
}
Expand Down
22 changes: 13 additions & 9 deletions src/lib/pmd/PmdEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,19 @@ abstract class AbstractPmdEngine extends AbstractRuleEngine {
this.logger.trace('No matching pmd target files found. Nothing to execute.');
return [];
}
const fh = new FileHandler();
const reportFile: string = await fh.tmpFileWithCleanup();
this.logger.trace(`About to run PMD rules. Targets: ${targetPaths.length}, Selected rules: ${selectedRules}`);

const stdout = await (await PmdWrapper.create({
await (await PmdWrapper.create({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to put the stdout in as a trace log (if it is non-empty) instead of just dropping it? It might help with debugging purposes in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. I'll do that.

targets: targetPaths,
rules: selectedRules,
rulePathsByLanguage,
supplementalClasspath
supplementalClasspath,
reportFile
})).execute();
const results = this.processStdOut(stdout);
const rawResults: string = await fh.readFile(reportFile);
const results = this.processOutput(rawResults);
this.logger.trace(`Found ${results.length} for PMD`);
return results;
} catch (e) {
Expand All @@ -167,21 +171,21 @@ abstract class AbstractPmdEngine extends AbstractRuleEngine {


/**
* stdout returned from PMD contains an XML payload that may be surrounded by other text.
* PMD output is represented as an XML payload.
* 'file' nodes from the XML are returned as rows of output to the user. 'configerror', 'error',
* and 'suppressedviolation' nodes are emitted as warnings from the CLI.
*/
protected processStdOut(stdout: string): RuleResult[] {
protected processOutput(output: string): RuleResult[] {
let results: RuleResult[] = [];

this.logger.trace(`Output received from PMD: ${stdout}`);
this.logger.trace(`Output received from PMD: ${output}`);

// Try to find the xml payload. It begins with '<?xml' and ends with '</pmd>'
const pmdEnd = '</pmd>';
const xmlStart = stdout.indexOf('<?xml');
const xmlEnd = stdout.lastIndexOf(pmdEnd);
const xmlStart = output.indexOf('<?xml');
const xmlEnd = output.lastIndexOf(pmdEnd);
if (xmlStart != -1 && xmlEnd != -1) {
const pmdXml = stdout.slice(xmlStart, xmlEnd + pmdEnd.length);
const pmdXml = output.slice(xmlStart, xmlEnd + pmdEnd.length);
const pmdJson = xml2js(pmdXml, {compact: false, ignoreDeclaration: true}) as Element;

const elements = pmdJson.elements[0].elements;
Expand Down
5 changes: 4 additions & 1 deletion src/lib/pmd/PmdWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ type PmdWrapperOptions = PmdSupportOptions & {
* are handled elsewhere).
*/
supplementalClasspath: string[];
reportFile: string;
};

export default class PmdWrapper extends PmdSupport {
Expand All @@ -22,12 +23,14 @@ export default class PmdWrapper extends PmdSupport {
private supplementalClasspath: string[];
private logger: Logger;
private initialized: boolean;
private readonly reportFile: string;

public constructor(opts: PmdWrapperOptions) {
super(opts);
this.targets = opts.targets;
this.rules = opts.rules;
this.supplementalClasspath = opts.supplementalClasspath;
this.reportFile = opts.reportFile;
}

protected async init(): Promise<void> {
Expand Down Expand Up @@ -56,7 +59,7 @@ export default class PmdWrapper extends PmdSupport {

const pmdCommandInfo: PmdCommandInfo = Controller.getActivePmdCommandInfo();
const classPathsForExternalRules: string[] = this.buildSharedClasspath().concat(this.supplementalClasspath);
const args: string[] = pmdCommandInfo.constructJavaCommandArgsForPmd(tmpPath, classPathsForExternalRules, this.rules);
const args: string[] = pmdCommandInfo.constructJavaCommandArgsForPmd(tmpPath, classPathsForExternalRules, this.rules, this.reportFile);

this.logger.trace(`Preparing to execute PMD with command: "${command}", args: "${JSON.stringify(args)}"`);
return [command, args];
Expand Down
12 changes: 6 additions & 6 deletions test/lib/pmd/PmdEngine.test.ts
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add a net-new test, because this behavior is a little hard to reliably and permanently test for. I can try to do that if people would like, though.

Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describe('Tests for BasePmdEngine and PmdEngine implementation', () => {
afterEach(() => {
Sinon.restore();
});
describe('processStdOut()', () => {
describe('processOutput()', () => {
it('Nodes that do not represent violations are filtered out of results', async () => {
// This file contains a `file`-type node and a mix of other node types that are direct children of the `pmd`
// node. The file nodes and the error nodes representing parser errors should be converted to violations.
Expand All @@ -47,7 +47,7 @@ describe('Tests for BasePmdEngine and PmdEngine implementation', () => {
const xml: string = await fileHandler.readFile(xmlPath);
expect(xml).to.not.be.null;

const results = (testPmdEngine as any).processStdOut(xml);
const results = (testPmdEngine as any).processOutput(xml);
expect(results).to.be.length(3, 'Should be three result entries');
expect(results[0].violations).to.be.length(13, 'Unexpected violation count in 1st entry');
expect(results[1].violations).to.be.length(1, 'Unexpected violation count in 2nd entry');
Expand Down Expand Up @@ -94,7 +94,7 @@ describe('Tests for BasePmdEngine and PmdEngine implementation', () => {
const xml: string = await fileHandler.readFile(xmlPath);
expect(xml).to.not.be.null;

await (testPmdEngine as any).processStdOut(xml);
await (testPmdEngine as any).processOutput(xml);
Sinon.assert.callCount(uxSpy, 3);
Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, expectedConfigError);
Sinon.assert.calledWith(uxSpy, EVENTS.WARNING_ALWAYS, expectedError);
Expand Down Expand Up @@ -193,19 +193,19 @@ describe('Tests for BasePmdEngine and PmdEngine implementation', () => {

describe('processStdout unusual cases', () => {
it('Empty stdout', async () => {
const results = await (testPmdEngine as any).processStdOut('');
const results = await (testPmdEngine as any).processOutput('');
expect(results).to.be.not.null;
expect(results).to.be.lengthOf(0);
});

it('Missing closing tag', async () => {
const results = await (testPmdEngine as any).processStdOut('<?xml blah blah blah');
const results = await (testPmdEngine as any).processOutput('<?xml blah blah blah');
expect(results).to.be.not.null;
expect(results).to.be.lengthOf(0);
});

it('Missing opening tag', async () => {
const results = await (testPmdEngine as any).processStdOut('blah blah blah</pmd>');
const results = await (testPmdEngine as any).processOutput('blah blah blah</pmd>');
expect(results).to.be.not.null;
expect(results).to.be.lengthOf(0);
});
Expand Down