Skip to content

Commit

Permalink
Addressing PR review issues
Browse files Browse the repository at this point in the history 
Add suppression support (#2435)

* Add suppression support

* Add incompatibility check and make suppressions non-null

Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>

Update releasehistory

fix couple test cases
  • Loading branch information
@yongyan-gh
yongyan-gh committed Feb 8, 2022
1 parent 0727702 commit 6306faf
Show file tree
Hide file tree
Showing 36 changed files with 2,177 additions and 1,356 deletions.
19 changes: 16 additions & 3 deletions src/ESLint.Formatter/sarif.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,11 @@ module.exports = function (results, data) {
console.log(err);
}
}
if (result.messages.length > 0) {

result.messages.forEach(message => {
const messages = result.suppressedMessages ? result.messages.concat(result.suppressedMessages) : result.messages;

if (messages.length > 0) {
messages.forEach(message => {

const sarifRepresentation = {
level: getResultLevel(message),
Expand Down Expand Up @@ -208,7 +210,7 @@ module.exports = function (results, data) {
}
if (message.column > 0) {
sarifRepresentation.locations[0].physicalLocation.region.startColumn = message.column;
};
}
}

if (message.source) {
Expand All @@ -220,6 +222,17 @@ module.exports = function (results, data) {
};
}

if (message.suppressions) {
sarifRepresentation.suppressions = message.suppressions.map(suppression => {
return {
kind: suppression.kind === "directive" ? "inSource" : "external",
justification: suppression.justification
}
});
} else {
sarifRepresentation.suppressions = [];
}

if (message.ruleId) {
sarifResults.push(sarifRepresentation);
} else {
Expand Down
172 changes: 158 additions & 14 deletions src/ESLint.Formatter/test/sarif.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ describe("formatter:sarif", () => {
describe("when passed no messages", () => {
const code = [{
filePath: sourceFilePath1,
messages: []
messages: [],
suppressedMessages: []
}];

it("should return a log with files, but no results", () => {
Expand All @@ -103,6 +104,94 @@ describe("formatter:sarif", () => {

describe("formatter:sarif", () => {
describe("when passed one message", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId
}],
suppressedMessages: []
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
assert.lengthOf(log.runs[0].results, 1);
assert.strictEqual(log.runs[0].results[0].level, "error");
assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].messages[0].message);
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.isDefined(log.runs[0].results[0].suppressions);
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
});
});

describe("when passed one suppressedMessage", () => {
const code = [{
filePath: sourceFilePath1,
messages: [],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
suppressions: [{ kind: "directive", justification: "foo" }]
}]
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
assert.lengthOf(log.runs[0].results, 1);
assert.strictEqual(log.runs[0].results[0].level, "error");
assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].suppressedMessages[0].message);
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.lengthOf(log.runs[0].results[0].suppressions, 1);
assert.strictEqual(log.runs[0].results[0].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
});
});

describe("when passed one suppressedMessage with multiple suppressions", () => {
const code = [{
filePath: sourceFilePath1,
messages: [],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
suppressions: [
{ kind: "directive", justification: "foo" },
{ kind: "directive", justification: "bar" }
]
}]
}];

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert.isDefined(log.runs[0].results[0].message);
assert.strictEqual(log.runs[0].results[0].message.text, code[0].suppressedMessages[0].message);
assert.lengthOf(log.runs[0].results[0].suppressions, 2);
assert.strictEqual(log.runs[0].results[0].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
assert.strictEqual(log.runs[0].results[0].suppressions[1].kind, "inSource");
assert.strictEqual(log.runs[0].results[0].suppressions[1].justification, code[0].suppressedMessages[0].suppressions[1].justification);
});
});

describe("when passed one message and no suppressedMessages array", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
Expand All @@ -114,7 +203,7 @@ describe("formatter:sarif", () => {

it("should return a log with one file and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));

assert(log.runs[0].artifacts[0].location.uri.startsWith(uriPrefix));
assert(log.runs[0].artifacts[0].location.uri, "/" + sourceFilePath1);
assert.isDefined(log.runs[0].results);
Expand All @@ -125,6 +214,8 @@ describe("formatter:sarif", () => {
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.startsWith(uriPrefix));
assert(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri.endsWith("/" + sourceFilePath1));
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.artifactLocation.index, 0);
assert.isDefined(log.runs[0].results[0].suppressions);
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
});
});
});
Expand All @@ -138,7 +229,8 @@ describe("formatter:sarif", () => {
message: "Unexpected value.",
ruleId: ruleid,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one rule", () => {
Expand All @@ -162,7 +254,8 @@ describe("formatter:sarif", () => {
message: "Unexpected value.",
ruleId: testRuleId,
line: 10
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location region has only a startLine", () => {
Expand All @@ -184,7 +277,8 @@ describe("formatter:sarif", () => {
ruleId: testRuleId,
line: 10,
column: 0
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line # and no column #", () => {
Expand All @@ -206,7 +300,8 @@ describe("formatter:sarif", () => {
ruleId: testRuleId,
line: 10,
column: 5
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -229,7 +324,8 @@ describe("formatter:sarif", () => {
line: 10,
column: 5,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -251,7 +347,8 @@ describe("formatter:sarif", () => {
severity: 2,
ruleId: testRuleId,
source: "getValue()"
}]
}],
suppressedMessages: []
}];

it("should return a log with one result whose location contains a region with line and column #s", () => {
Expand All @@ -264,6 +361,39 @@ describe("formatter:sarif", () => {
});
});



describe("formatter:sarif", () => {
describe("when passed one message and one suppressedMessage", () => {
const code = [{
filePath: sourceFilePath1,
messages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
source: "getValue()"
}],
suppressedMessages: [{
message: "Unexpected value.",
severity: 2,
ruleId: testRuleId,
source: "getValue()",
suppressions: [{ kind: "directive", justification: "foo" }]
}]
}];

it("should return a log with two results, one of which has suppressions", () => {
const log = JSON.parse(formatter(code, rules));

assert.lengthOf(log.runs[0].results, 2)
assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 1);
assert.strictEqual(log.runs[0].results[1].suppressions[0].kind, "inSource");
assert.strictEqual(log.runs[0].results[1].suppressions[0].justification, code[0].suppressedMessages[0].suppressions[0].justification);
});
});
});

describe("formatter:sarif", () => {
describe("when passed two results with two messages each", () => {
const ruleid1 = "no-unused-vars";
Expand Down Expand Up @@ -291,7 +421,8 @@ describe("formatter:sarif", () => {
line: 10,
column: 5,
source: "doSomething(thingId)"
}]
}],
suppressedMessages: []
},
{
filePath: sourceFilePath2,
Expand All @@ -305,7 +436,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid3,
line: 42
}]
}],
suppressedMessages: []
}];

it("should return a log with two files, three rules, and four results", () => {
Expand Down Expand Up @@ -385,6 +517,11 @@ describe("formatter:sarif", () => {
assert.strictEqual(log.runs[0].results[2].locations[0].physicalLocation.region.startLine, 18);
assert.isUndefined(log.runs[0].results[2].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[2].locations[0].physicalLocation.region.snippet);

assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 0);
assert.lengthOf(log.runs[0].results[2].suppressions, 0);
assert.lengthOf(log.runs[0].results[3].suppressions, 0);
});
});
});
Expand All @@ -404,7 +541,8 @@ describe("formatter:sarif", () => {
};
const code = [{
filePath: sourceFilePath1,
messages: [ ]
messages: [],
suppressedMessages: []
},
{
filePath: sourceFilePath2,
Expand All @@ -418,7 +556,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid3,
line: 42
}]
}],
suppressedMessages: []
}];

it("should return a log with two files, two rules, and two results", () => {
Expand Down Expand Up @@ -464,6 +603,9 @@ describe("formatter:sarif", () => {
assert.strictEqual(log.runs[0].results[0].locations[0].physicalLocation.region.startLine, 18);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.startColumn);
assert.isUndefined(log.runs[0].results[0].locations[0].physicalLocation.region.snippet);

assert.lengthOf(log.runs[0].results[0].suppressions, 0);
assert.lengthOf(log.runs[0].results[1].suppressions, 0);
});
});
});
Expand All @@ -476,7 +618,8 @@ describe("formatter:sarif", () => {
message: "Internal error.",
severity: 2,
// no ruleId property
}]
}],
suppressedMessages: []
}];

it("should return a log with no rules, no results, and a tool execution notification", () => {
Expand Down Expand Up @@ -526,7 +669,8 @@ describe("formatter:sarif", () => {
message: "Custom error.",
ruleId: ruleid,
line: 42
}]
}],
suppressedMessages: []
}];
it("should return a log with one file, one rule, and one result", () => {
const log = JSON.parse(formatter(code, { rulesMeta: rules }));
Expand Down
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415)
* BREAKING: `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. [#2437](https://github.com/microsoft/sarif-sdk/pull/2437)
* BUGFIX: Fix `ArgumentException` when `--recurse` is enabled and two file target specifiers generates the same file path. [#2438](https://github.com/microsoft/sarif-sdk/pull/2438)
* FEATURE: Add `--sort-results` argument to `rewrite` command to get sorted SARIF results. [#2422](https://github.com/microsoft/sarif-sdk/pull/2422)

## **v2.4.12** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.12) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.12) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.12) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.12) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.12)

Expand Down
Loading

0 comments on commit 6306faf

Please sign in to comment.