Skip to content
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

Add SARIF support #1221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shaopeng-gh
Copy link

@shaopeng-gh shaopeng-gh commented Nov 2, 2022

Add SARIF support, for #1045
usage: -f sarif
@coliff

@github-actions github-actions bot added cli Relates to HTMLHint's command-line interface dependencies Pull requests that update a dependency file test labels Nov 2, 2022
@coliff coliff requested review from coliff and nschonni November 2, 2022 08:57
@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #1221 (a05d02b) into master (a430c97) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1221   +/-   ##
=======================================
  Coverage   99.07%   99.07%           
=======================================
  Files           2        2           
  Lines        1627     1627           
  Branches      309      309           
=======================================
  Hits         1612     1612           
  Misses         15       15           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c22c21d...a05d02b. Read the comment docs.

child.stdout.on('data', function (stdout) {
expect(stdout).not.toBe('')

if (os.platform() !== 'darwin') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have problem with this test running in macOS,
at first I was using the exact same way ChildProcess.exec, copied from json.spec.js. It seems macOS will truncate because sarif is too long.
So I tried to change to ChildProcess.spawn, seems like still have same issue. Not familiar with NodeJS, Let me know.
This looks like related:
nodejs/node#19218

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I didn't land it, but could this use the Jest snapshoting to work around it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking,
the issue was we are comparing the output against a pre-saved file,
in macOS only, the stdout seems have limit, and will lose part of the json,
the error was:
SyntaxError: Unexpected end of JSON input at at JSON.parse (<anonymous>)
> 34 | const jsonStdout = JSON.parse(stdout)

the tests for Linux and Windows does not have this issue and pass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the above thread in nodejs repo, I can see we had a same issue before. #442
Our fix was trim down the size of the test. I can try to do the same to fix this as well so the test can be ran in macOS.

"integrity": "sha512-Pzr3rol8fvhG/oJjIq2NTVB0vmdNNlz22FENhhPojYRZ4/ee08CfK4YuKmuL54V9MLhI1kpzxfOJ/63LzmZzDg==",
"dev": true,
"dependencies": {
"@types/sarif": "^2.1.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: the types probably don't need to be a dependency, but that's an upstream issue

child.stdout.on('data', function (stdout) {
expect(stdout).not.toBe('')

if (os.platform() !== 'darwin') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I didn't land it, but could this use the Jest snapshoting to work around it?

@coliff coliff added the keep-unstale The issue will not be marked as stale by the stale-bot label Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Relates to HTMLHint's command-line interface dependencies Pull requests that update a dependency file keep-unstale The issue will not be marked as stale by the stale-bot test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants