Conversation
This reverts commit 006cc8c.
aeisenberg
force-pushed
the
aeisenberg/codeSnippet-handling
branch
from
19a0bc7 to
6fa0227
Compare
…/codeSnippet-handling
aeisenberg
force-pushed
the
aeisenberg/analysis-results-on-restart
branch
from
de06ed1 to
8daa92a
Compare
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Thanks for extracting this out into its own PR, it's much easier to review!
Changes generally looking good to me, just a few small comments.
| @@ -1,5 +1,4 @@ | |||
| import * as path from 'path'; | |||
| import * as fs from 'fs-extra'; | |||
There was a problem hiding this comment.
What's going on here? Is this a bad merge?
There was a problem hiding this comment.
Yes...it's fixed in main, but since this PR doesn't target main, I need to have the fix here as well.
| }) => { | ||
|
|
||
| const code = codeSnippet.text.split('\n'); | ||
| const code = codeSnippet?.text.split('\n') || []; |
There was a problem hiding this comment.
What do you think about returning early if the codeSnippet is undefined, instead of having to deal with it throughout? E.g.
if (!codeSnippet) {
return (
<Container>
<TitleContainer>
<Link href={titleFileUri}>{fileLink.filePath}</Link>
</TitleContainer>
</Container>
}(and perhaps extract the title stuff in its own component (within the function) so it can be re-used.
I think that makes clearer what should happen if code snippet is not present. But that's ofc just my opinion 😄
There was a problem hiding this comment.
Yes...good idea.
| await arm.downloadAnalysisResults(remoteQueryResult0.analysisSummaries[1], () => Promise.resolve()); | ||
|
|
||
| expect(await (arm as any).isDownloadedNotInMemory(remoteQueryResult0.analysisSummaries[0])).to.be.true; | ||
| expect(await (arm as any).isAnalysisDownloadedNotInMemory(remoteQueryResult0.analysisSummaries[0])).to.be.true; |
There was a problem hiding this comment.
These look like artifacts of a bad merge
| expect(actualCodeSnippet).to.deep.equal(expectedCodeSnippet); | ||
| }); | ||
|
|
||
| it('should not return errors for result locations with no contextRegion', () => { |
There was a problem hiding this comment.
| it('should not return errors for result locations with no contextRegion', () => { | |
| it('should use highlightedRegion for result locations with no contextRegion', () => { |
| expect(msg.startsWith('Error when processing SARIF result')).to.be.true; | ||
| } | ||
|
|
||
| function expectNoParsingError(result: { errors: string[] | undefined }) { |
There was a problem hiding this comment.
The result returned by extractAnalysisAlerts is always guaranteed to return errors. I'm not sure why we're dealing with undefined here.
Isn't the check just expect(result.errors.length).to.equal(0)?
There was a problem hiding this comment.
Yes...undefined isn't an option here. I'll change that.
I was using expect(array.length, array.join()).to.equal(0); so that the second parameter to expect would print outt he actual array if there was an error. It was helpful earlier on while I was writing the test. And I decided to keep it since it's generally useful.
…/codeSnippet-handling
Also, simplify sarif tests.
2 participants
This reverts commit 006cc8c.
Allows for SARIF that has regions but no code snippets.
Checklist
ready-for-doc-reviewlabel there.