-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: add apex code coverage reporters #284
Conversation
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.
Did a quick first pass at the easy questions first; will dive more into the essentials of the change tomorrow!
packages/plugin-apex/my/path/to/dir/test-result-707xx0000AUS2gH-junit.xml
Outdated
Show resolved
Hide resolved
@@ -11,6 +11,7 @@ | |||
"outDir": "./lib", | |||
"preserveConstEnums": true, | |||
"strict": true, | |||
"skipLibCheck": true, |
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.
Was this change necessary for any particular dependency?
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.
Having issue with @oclif/parser
$ shx rm -rf lib && tsc -b
node_modules/@salesforce/command/node_modules/@oclif/command/node_modules/@oclif/config/node_modules/@oclif/parser/lib/index.d.ts:16:60 - error TS2344: Type 'TArgs' does not satisfy the constraint 'any[]'.
Type '{ [name: string]: string; }' is missing the following properties from type 'any[]': length, pop, push, concat, and 26 more.
16 }>(argv: string[], options: Input<TFlags>): Output<TFlags, TArgs>;
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.
mshanemc or other keepers of salesforce/command, do you have any thoughts on why this might be an issue? -> Update: My apologies - I didn't realize you were on the CLI team and also a keeper of this command 🤦🏻♀️ we're fine to keep this here for now if we need; we've had it added on the vscode side too.
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.
@peternhale do you have an idea of the documentation changes we might want to add with this? I've added our doc writer @sbudhirajadoc to make sure we update our docs to match the new functionality here, for instance on our apex Commands reference page.
@randi274 For these changes, probably on API documentation for the new CoverageReporter.
I was not planning on modifying anything in plugin-apex. I know adding coverage is a goal, but outside the scope of the work I am doing for the customer.
@peternhale do you have an idea of the documentation changes we might want to add with this? I've added our doc writer @sbudhirajadoc to make sure we update our docs to match the new functionality here, for instance on our apex Commands reference page. |
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.
Just a few more requests to flesh out code coverage - after that and I review with @AnanyaJha I think we'll be good though!
packages/apex-node/test/coverageReporters/coverageReporter.test.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,2 @@ | |||
# Attribution for Apex Files used for testing |
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.
This looks great - thanks so much for adding it!
@@ -0,0 +1,14 @@ | |||
/* |
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.
Last review question on this @peternhale, more of a semantic one - could this be included under src/reporters? Should it be? Or do you feel this is distinct enough to merit the separation? Just thinking for future maintenance, we may wonder why we have two different reporter directories.
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.
I had considered using reporters dir, but felt like it was a bit too different. I an on the fence, so I don't mind moving it to reporters.
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.
Linked plugin locally and:
✅ Ran test run. Confirmed junit, tap, human, and json reporters still working as expected.
✅ Did successful spot check on test report, log list, and log get commands.
What does this PR do?
Add code coverage reporters as provided via istanbul APIs
What issues does this PR fix or reference?
@W-11025210@
Functionality Before
Does not exist
Functionality After
Add CoverageReporter class, which provides interface to create code coverage reports from ApexCodeCoverageAggregate or ApexCodeCoverage data. The class consumes istanbul API that generate coverage reports.
Report types included are: see istanbul for details
lcov and text-lcov have been exclude. Report text-lcov writes to the console exclusively and lcov duplicates html and locvonly.