- Notifications
You must be signed in to change notification settings - Fork427
fix: support projects with namespace but org without replay debugger W-19604488#6579
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
packages/salesforcedx-vscode-apex/src/views/testOutlineProvider.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| publicerrorMessage:string=''; | ||
| publicstackTrace:string=''; | ||
| publicoutcome='Not Run'; | ||
| publicreadonlyoutcome:TestOutcome='Skip'; |
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.
set viaupdateOutcome instead of having to set it and then call updateOutcome on it.
Uh oh!
There was an error while loading.Please reload this page.
| Method | ||
| } | ||
| exportclassApexTestRunner{ |
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.
didn't need to be a class. Only 1 method was using the outlineProvider.
that event emitter was even sillier (letting one method call another, I think to get around somthing not being async).
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| import{DebugProtocol}from'@vscode/debugprotocol'; | ||
| exportclassBreakpointUtil{ | ||
| privatestaticinstance:BreakpointUtil; |
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.
a bunch of this was either completely unused or used only for testing.
exposing the 2 maps simplifies things a lot
| } | ||
| } | ||
| exportconstbreakpointUtil=newBreakpointUtil(); |
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.
you can instantiate a class and export that instance instead of doing all the singleon pattern getInstance boilerplate
| publicsetOverlaySuccessResult(overlaySuccessResult:ApexExecutionOverlayResultCommandSuccess):void{ | ||
| this.overlaySuccessResut=overlaySuccessResult; | ||
| } | ||
| exporttypeApexHeapDump={ |
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.
you don't need a class just to make some immutable props.
if you see a getter/setter on the same prop, without any fancy mutation, that's just a public mutable property wasted code
| import{LogContext}from'./logContext'; | ||
| import{substringUpToLastPeriod,substringFromLastPeriod}from'./logContextUtil'; | ||
| exportclassHeapDumpService{ |
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 ones ugly so I stopped short.
| * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
| */ | ||
| exportclassLogContextUtil{ |
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 was passed around, mocked, and attached to other classes but shouldn't have been a class
| }; | ||
| exporttypeApexExecutionOverlayResultCommandSuccess={ | ||
| exporttypeApexExecutionOverlayResult={ |
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.
renamed to match tooling API types
| } | ||
| })); | ||
| jest.mock('../../../src/core/logContextUtil',()=>({ |
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.
how to mock a standalone fn so you don't need classes
| symbols:['theInt'], | ||
| value:{ | ||
| value:5 | ||
| constheapdump:ApexHeapDump={ |
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.
the mocks were not very accurate, which was why theassertions were needed. AI is good for realistic mocks, keeps working until the compiler is satisfied.
why not test with realistic data?
| consttyperefMapping:Map<string,string>=newMap(); | ||
| typerefMapping.set('foo','file:///foo.cls'); | ||
| typerefMapping.set('bar','file:///bar.cls'); | ||
| constlineNumberMapping:Map<string,number[]>=newMap([ |
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.
maps have a ctor
| }); | ||
| it('ApexExecutionOverlayResultCommand GET REST call with parse-able heapdump success result',async()=>{ | ||
| overlayResultCommand=newApexExecutionOverlayResultCommand(heapdumpKey); |
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.
mock a json server response, the parse the mocked json and make sure JSON.parse worked? that's a dumb test.
| } | ||
| } | ||
| exportclassEmptyPreCheckerimplementsPreconditionChecker{ |
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.
unused
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 pkg just has a bit of cleanup
| returntestResult.codecoverage??testResult.coverage.coverage; | ||
| }; | ||
| constisApexMetadata=(filePath:string):boolean=>IS_CLS_OR_TRIGGER.test(filePath); |
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.
string methods > regex
| location:vscode.Location; | ||
| }; | ||
| exportclassApexLSPConverter{ |
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.
no reason for a class
| awaitactivationTracker.markActivationStop(); | ||
| }; | ||
| exportconstretrieveLineBreakpointInfo=async():Promise<boolean>=>{ |
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.
moved out of index to its own file
| privatechildren:BaseNode[]=[]; | ||
| publiccheckpointOverlayAction:ApexExecutionOverlayAction; | ||
| privateuri:string; | ||
| privateenabled:boolean; |
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.
nothing was using the actionObjectId. It was set but never read.
What does this PR do?
primary
other
What issues does this PR fix or reference?
@W-19604488@
QA both the WI issue and some general "around the test runner"