Use sarif parser for reopened results by angelapwen · Pull Request #1457 · github/vscode-codeql

angelapwen · 2022-08-09T14:38:38Z

When we attempt to re-open query results, instead of using the streaming SARIF parser written in #1004, we used JSON.parse(). This couldn't handle large SARIF files and we ran into an error.

This change uses the streaming SARIF parser instead. Note that this is a draft pull request as it is untested locally. cc @aschackmull (sorry, wrong Anders tagged at first!)

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

extensions/ql-vscode/src/query-results.ts

angelapwen · 2022-08-09T15:22:43Z

Looks like there are some genuine unit test errors on the method I changed so this will need more 🕵️ work

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

aeisenberg · 2022-10-13T16:18:21Z

extensions/ql-vscode/src/query-results.ts

+  let res;
  if (await fs.pathExists(interpretedResultsPath)) {
-    return { ...JSON.parse(await fs.readFile(interpretedResultsPath, 'utf8')), t: 'SarifInterpretationData' };
+    res = await sarifParser(interpretedResultsPath);


Just a thought....if this function throws an error because the file is not parseable, should we delete the file and re-run cli.interpretBqrsSarif?

Possibly, we can do this as a followup.

Hm, that's a good point 👍 I'll leave this comment up for now, while we work out the last invalid SARIF test issue..

extensions/ql-vscode/src/sarif-parser.ts

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

aeisenberg · 2022-10-19T22:59:27Z

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

-    expect(results3).to.deep.eq({ a: 6, t: 'SarifInterpretationData' });
+    afterEach(async () => {
+      sandbox.restore();
+      await safeDel(interpretedResultsPath);


I'm actually seeing sometimes that safeDel isn't so safe. It might be causing some hangs in the cli-integration tests. Can you use the del function instead?

Suggested change

await safeDel(interpretedResultsPath);

await del(interpretedResultsPath);

And add this import to the imports at the top of the file:

import * as del from 'del';

Actually the test was failing in CI (not locally) when I used del — it said that the file wasn't found when it tried to delete it. See https://github.com/github/vscode-codeql/actions/runs/3285438706/jobs/5412531729

1) query-results interpretResultsSarif "after each" hook for "should use sarifParser on a valid small SARIF file": Error: Cannot delete files/directories outside the current working directory. Can be overridden with the `force` option. at safeCheck (node_modules/del/index.js:37:9) at mapper (node_modules/del/index.js:73:4) at /home/runner/work/vscode-codeql/vscode-codeql/extensions/ql-vscode/node_modules/p-map/index.js:57:28 at processTicksAndRejections (node:internal/process/task_queues:96:5)

I'm going to remove the del call entirely and see if it works, given that I am over-writing the existing file when the WriteStream is created.

Removing del altogether worked on ubuntu. There is still a failure in this same test in Windows

[main 2022-10-20T17:11:53.979Z] Extension host with pid 6888 exited with code: 134, signal: null. Test runner caught exception (Failed) Exit code: 134 Done Tried running suite 3 time(s), still failed, giving up. Error: Process completed with exit code 1.

that isn't very descriptive. Exit code 134 seems to be a SIGABRT. I seem to remember there being some intricacies around Windows and file I/O — maybe I'm hitting one of them here?

aeisenberg · 2022-10-19T23:01:01Z

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

+
+    it('should interpretResultsSarif', async function() {
+      // up to 2 minutes per test
+      this.timeout(2 * 60 * 1000);


Can you move this (and the other) timeout to the describe block above? You only need to set the timeout once up there.

I get an Object is possibly undefined error when I move it to the describe block or the beforeEach block — is there a way to get around that error?

Figured out how move this to the beforeEach hook and pushed up.

Nevermind, that didn't work... had to revert

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

aeisenberg · 2022-10-20T19:24:01Z

I can't tell why the windows tests are failing. It seems to be failing consistently. If you have access to a windows box, maybe try running the tests there.

angelapwen · 2022-10-20T20:28:26Z

I can't tell why the windows tests are failing. It seems to be failing consistently. If you have access to a windows box, maybe try running the tests there.

I don't have one, but will 🍐 with Dave tomorrow on this!

aeisenberg · 2022-10-21T17:56:17Z

Now, looks like the tests are failing because interpreted.json is trying to be deleted, but a lock on it is being held by another process or something. It has to do with the safeDel method. For some reason, it is not able to be deleted and it looks like the tests can't even read it.

Not a pleasing solution, but perhaps, you can use a different name for interpreted results file for each test. This way, you won't need to delete it in afterEach and you can be sure you can read it in each test.

angelapwen · 2022-10-21T20:12:45Z

Now, looks like the tests are failing because interpreted.json is trying to be deleted, but a lock on it is being held by another process or something. It has to do with the safeDel method. For some reason, it is not able to be deleted and it looks like the tests can't even read it.

Not a pleasing solution, but perhaps, you can use a different name for interpreted results file for each test. This way, you won't need to delete it in afterEach and you can be sure you can read it in each test.

Dave and I just came up with something like this in pairing too. It looked like there was a separate issue with Windows Defender that needed a sleep after the file was written (because it couldn't be opened while Windows Defender was scanning it), so we added 10s sleep as well. After that we discovered the problem in the final test where it wasn't able to be deleted/read.

I've renamed the file for just the last test to see if it passes for now (can't reproduce locally without a Windows machine). I can also rename the others for consistency.

This reverts commit 262fbee.

aeisenberg · 2022-10-21T21:59:15Z

Not sure if you've tried this already, but you can launch the test version of vscode with --verbose and --log trace` to get some more info while you are running. Augment the CLI options in getLaunchArgs.

angelapwen · 2022-10-24T17:34:20Z

Phew, renaming the path worked 😸 so this PR is finally ready for re-review

aeisenberg

Great job sticking with this and working through all these frustrating details. It seemed like a simple task, but there was a lot of subtlety with it.

Use sarif parser for reopened results

c13319d

adityasharad reviewed Aug 9, 2022

View reviewed changes

extensions/ql-vscode/src/query-results.ts Show resolved Hide resolved

angelapwen mentioned this pull request Aug 9, 2022

Handle large (>4GB) SARIF results files on reopen #1455

Closed

angelapwen added 2 commits August 15, 2022 09:49

Bump unit test timeout to 3 min

b3d7e78

Bump unit test timeout to 5 min

c37c5bf

angelapwen force-pushed the handle-sarif-reopen branch from acb95db to c37c5bf Compare August 15, 2022 17:10

angelapwen added 7 commits October 11, 2022 12:04

Merge branch 'main' into handle-sarif-reopen

f0d0017

Use sarif parser for reopened results

c5a816d

Remove extra import statement

c1b3ee1

Exit parser when invalid SARIF is parsed

2b6dd6b

Reset test timeout to 2 min

169a88a

Add tests for valid and invalid small SARIF files

fc32971

Add large SARIF file tests

ca8f930

angelapwen commented Oct 12, 2022

View reviewed changes

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts Outdated Show resolved Hide resolved

Delete large valid SARIF before invalid test

8c92a19

angelapwen commented Oct 13, 2022

View reviewed changes

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts Outdated Show resolved Hide resolved

aeisenberg reviewed Oct 13, 2022

View reviewed changes

angelapwen added 6 commits October 13, 2022 12:36

Use del rather than unlink

438607d

Update imports

a07e549

Close write stream

9fe8fd8

Improve SARIF parser and interpreter tests

d656db0

Move interpretedResultsPath to before hook

dcc51cc

Use safeDel rather than del

badbed9

angelapwen marked this pull request as ready for review October 19, 2022 22:50

angelapwen requested a review from a team as a code owner October 19, 2022 22:50

aeisenberg reviewed Oct 19, 2022

View reviewed changes

angelapwen added 2 commits October 20, 2022 08:57

Write valid JSON to large valid SARIF test

6bd649a

Remove file deletion in after block

05c6efe

Use safeDel in after hook

f75f0b7

angelapwen force-pushed the handle-sarif-reopen branch from f75f0b7 to 36e7a41 Compare October 21, 2022 00:16

angelapwen requested a review from a team as a code owner October 21, 2022 00:16

angelapwen force-pushed the handle-sarif-reopen branch from 36e7a41 to f75f0b7 Compare October 21, 2022 00:33

Attempt 1000 iterations for large tests

f7dc7b7

angelapwen added 5 commits October 21, 2022 13:09

Sleep 10ms in tests on Windows

50b3109

Add missing import

fbe0b98

Change stream listeners to close rather than finish

93bd94c

Write 1mil times for SARIF files

86eaf9d

Write to a new path for last test

b849fa9

angelapwen added 2 commits October 21, 2022 13:20

Refactor timeout to before hook

262fbee

Revert "Refactor timeout to before hook"

75c8aa3

This reverts commit 262fbee.

angelapwen added 3 commits October 21, 2022 15:12

Rename invalid results path

f37bf65

Update results path var name

ca0c863

Add comment explanation

671f0e2

angelapwen requested a review from aeisenberg October 24, 2022 17:33

aeisenberg approved these changes Oct 24, 2022

View reviewed changes

angelapwen merged commit 63a5021 into github:main Oct 24, 2022

angelapwen deleted the handle-sarif-reopen branch October 24, 2022 19:31

	await safeDel(interpretedResultsPath);
	await del(interpretedResultsPath);

Conversation

angelapwen commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

angelapwen commented Aug 9, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg commented Oct 20, 2022

Uh oh!

angelapwen commented Oct 20, 2022

Uh oh!

aeisenberg commented Oct 21, 2022

Uh oh!

angelapwen commented Oct 21, 2022

Uh oh!

aeisenberg commented Oct 21, 2022

Uh oh!

angelapwen commented Oct 24, 2022

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

angelapwen commented Aug 9, 2022 •

edited

Loading