vscode-codeql icon indicating copy to clipboard operation
vscode-codeql copied to clipboard

Use sarif parser for reopened results

Open angelapwen opened this issue 3 years ago • 1 comments

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.
  • [x] Issues have been created for any UI or other user-facing changes made by this pull request.
  • [x] [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.

angelapwen avatar Aug 09 '22 14:08 angelapwen

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

angelapwen avatar Aug 09 '22 15:08 angelapwen

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.

aeisenberg avatar Oct 20 '22 19:10 aeisenberg

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!

angelapwen avatar Oct 20 '22 20:10 angelapwen

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.

aeisenberg avatar Oct 21 '22 17:10 aeisenberg

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.

angelapwen avatar Oct 21 '22 20:10 angelapwen

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.

aeisenberg avatar Oct 21 '22 21:10 aeisenberg

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

angelapwen avatar Oct 24 '22 17:10 angelapwen