-
Notifications
You must be signed in to change notification settings - Fork 200
Fix large sarif handling #1004
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
Fix large sarif handling #1004
Conversation
0c07819
to
59682d4
Compare
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 is looking great. A few comments to clean things up. Since this is new behaviour, I'd like to see some unit tests for this. We can chat tomorrow about this.
I think the test failures are because you are using an older version of npm. Could you try upgrading to version 8? And then try re-running npm install. |
Actually, try rebasing on this PR #1003 The problem is that for some reason, fsevents is not marked as optional. |
|
||
it('should throw an error if the file fails to parse', async () => { | ||
const result = await CodeQLCliServer.parseSarif(__dirname + '/data/sarif/invalidSarif.sarif'); | ||
await expect(result).to.rejectedWith(/Error: Parsing output of interpretation failed: /); |
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.
Semantics are the same, but reads a little nicer.
await expect(result).to.rejectedWith(/Error: Parsing output of interpretation failed: /); | |
await expect(result).to.be.rejectedWith(/Error: Parsing output of interpretation failed: /); |
describe.only('cliServerTests', () => { | ||
it('should parse a valid SARIF file', async () => { | ||
const result = await CodeQLCliServer.parseSarif(__dirname + '/data/sarif/validSarif.sarif'); | ||
expect(result.runs.length).to.eq(1); |
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 think you want a different expectation here.
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 updated this. Hopefully it's a better test.
9369f77
to
fe40ebf
Compare
Authored by: Marc Jaramillo [email protected] Authored by: Musab Guma'a [email protected]
fe40ebf
to
ca5e5e2
Compare
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 promising! Some suggestions on organisation.
extensions/ql-vscode/src/vscode-tests/no-workspace/sarifParser.test.ts
Outdated
Show resolved
Hide resolved
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.
Nice work!
…s (OFBIZ-12361) According to github/vscode-codeql#1004 github/vscode-codeql#735 this should now work. Let's try...
…s (OFBIZ-12361) According to github/vscode-codeql#1004 github/vscode-codeql#735 this should now work. Let's try...
Replace this with a description of the changes your pull request makes.
Checklist
@github/docs-content-codeql
has been cc'd in all issues for UI or other user-facing changes made by this pull request.