Add SARIF schema test #105
Conversation
facebook-github-bot
added
the
CLA Signed
| @@ -289,6 +289,7 @@ def navigate_trace_frames( | |||
| callee_port=trace_frame.callee_port, | |||
| caller="", | |||
| caller_port="", | |||
| filename="", | |||
There was a problem hiding this comment.
to conform with the schema definition
| @@ -48,7 +48,7 @@ | |||
|
|
|||
| class SARIF: | |||
| version: str = "2.1.0" | |||
| schema: str = "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json" # noqa | |||
There was a problem hiding this comment.
the old schema is actually had a broken regex and it was fixed in the codeQL schema see this commit github/codeql-action@9824588
| if len(trace_tuples) == 0: | ||
| return [] |
There was a problem hiding this comment.
to conform with the schema definition
|
I think the failing test is just a flaky test. I don't think the failure is from this PR |
|
/cc @arthaud not sure if you guys missed this PR :D |
| import jsonschema | ||
| import requests | ||
|
|
||
| from sapp.sarif import SARIF |
There was a problem hiding this comment.
Below we use relative imports and here we use an absolute import, is this intentional? If not then let's try to be consisent.
| output = sarif.to_json() | ||
| output = json.loads(output) | ||
| try: | ||
| response = requests.get(SARIF.schema) |
There was a problem hiding this comment.
Not a big fan of tests pulling random stuff from the internet. Maybe we could just push that file in the repository?
| output = sarif.to_json() | ||
| output = json.loads(output) |
There was a problem hiding this comment.
sarif.to_json() does a json.dumps internally, so this is doing a back-and-forth between the python representation and a string. It would be nice to avoid this, but I guess that's not a big deal for a test..
| sarif = SARIF("mariana-trench", session, set(issues)) | ||
| output = sarif.to_json() | ||
| output = json.loads(output) | ||
| try: | ||
| response = requests.get(SARIF.schema) | ||
| response.raise_for_status() | ||
| schema = response.json() | ||
| jsonschema.Draft202012Validator(schema).validate(output) | ||
| except Exception as e: | ||
| print(f"Error downloading schema: {e}") | ||
| raise e |
There was a problem hiding this comment.
maybe move that in a function, since it's used twice.
| input_return_map = { | ||
| (session, issues[0].issue_instance_id, TraceKind.POSTCONDITION): [ | ||
| source_frames_query_results[0], | ||
| source_frames_query_results[1], | ||
| ], | ||
| (session, issues[0].issue_instance_id, TraceKind.PRECONDITION): [ | ||
| sink_frames_query_results[0], | ||
| sink_frames_query_results[1], | ||
| ], | ||
| } |
There was a problem hiding this comment.
Why is this necessary? From my understanding, since we use self.fakes.precondition/postcondition, those should be in the database and initial_frames should find those?
Pre-submission checklist
black .usort format .flake8pip install -r requirements-dev.txtand completed the following:./scripts/run-tests.shand made sure all tests are passingSummary
This PR adds initial tests to SARIF output from SAPP. Currently the SARIF output has no test coverage at all. This API add a schema test that the output SARIF passes the defined schema.
Test Plan
and running the specific new tests