@W-14980290@: Implement Pmd7CommandInfo to allow us to call pmd7 jars #1352
Conversation
| // TODO: We should remove this instantiation of new Ux in favor of possibly a new method on Display | ||
| new Ux({jsonEnabled: jsonEnabled}) | ||
| .styledObject(rule, ['name', 'engine', 'runWith', 'isPilot', 'enabled', 'categories', 'rulesets', 'languages', 'description', 'message']); | ||
| private displayStyledRule(rule: DescribeStyledRule): void { |
There was a problem hiding this comment.
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.
| column: occ.attributes.column as number, | ||
| endLine: occ.attributes.endline as number, | ||
| endColumn: occ.attributes.endcolumn as number, | ||
| line: Number(occ.attributes.line), |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| export class Pmd7CommandInfo implements PmdCommandInfo { |
There was a problem hiding this comment.
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%.
| // be OR'd together in this property. | ||
| defaultConfig?: ESRuleConfigValue; | ||
| url?: string; | ||
| message?: string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For example (even before my changes):
sf scanner rule describe -n ApexCRUDViolation
Would give a bunch of information including:
message: Validate CRUD permission before SOQL/DML operation or enforce user mode
| const pathToCodeForCpd = path.join(codeFixturesDir, 'cpd'); | ||
|
|
||
|
|
||
| describe("Tests for RunAction", () => { |
There was a problem hiding this comment.
These new tests are practically end to end tests except that they do not invoke the scanner - but the action directly. Also, I removed the output processing since that is all tested separetely.
Eventually I believe we will be moving most of our end to end tests to this format - leaving only a few actual end to end tests behind to confirm that we are wired up correctly to the cli and the output processors.
There was a problem hiding this comment.
This looks good - thanks for adding them.
stephen-carter-at-sf
force-pushed
the
d/W-14980290
branch
from
6e67985 to
3fa79c1
Compare
No description provided.