chore: Added tests for IaC JSON test output #3492
Conversation
ofekatr
force-pushed
the
chore/add-tests-for-test-json-output
branch
from
2fc9776
to
4e82244
Compare
ofekatr
force-pushed
the
chore/add-tests-for-test-json-output
branch
3 times, most recently
from
a26b8f5
to
758d996
Compare
ofekatr
force-pushed
the
chore/add-tests-for-test-json-output
branch
12 times, most recently
from
614a180
to
0d999b6
Compare
There was a problem hiding this comment.
Sorry it took me some time to get to this! I left some comments. I tried a few files and could not get some of the fields in any of my example runs. which files did you use? It's ok as the fields are not required but I'm just wondering which cases they cover.
Should we also cover the --experimental run with a similar test as we are already on it and we are building that too?
| "projectId": { | ||
| "type": "string" |
There was a problem hiding this comment.
I can't see this on any of my example runs :(
There was a problem hiding this comment.
This value should be populated when Share Results is used to create a project on the platform.
The value represents the public ID of the project that was created.
| "gitRemoteUrl": { | ||
| "type": "string" |
There was a problem hiding this comment.
this value is used in share results when we need to save the git remote url of the git repo that we are scanning.
the main use case is for contributing developers.
| "title": { | ||
| "type": "string" | ||
| }, | ||
| "description": { |
There was a problem hiding this comment.
Can't find this in any of my example runs. it's ok as it's not requred, I'd be interested to see in which policies it's getting generated.
There was a problem hiding this comment.
Scanning snyk-labs/infrastructure-as-code-goof, description was usually not set, but was sometimes set as an empty string.
Doing a search in snyk/cloud-config-opa-policies, you can see that some policies have the top-level description set as "", e.g., SNYK-CC-AWS-50.
It seems like this value is not being used since it's empty, but it exists in the output none the less.
| "type": { | ||
| "type": "string" |
There was a problem hiding this comment.
type does not exist in all policies. but I think it's fine here as you don't have it required 👍 just mentioning it.
There was a problem hiding this comment.
Just ran the following command in snyk/cli/test/fixtures/iac/cloudformation:
snyk iac test ./test/fixtures/iac/cloudformation --json | jq '[.[].infrastructureAsCodeIssues[] | keys | select(contains(["type"]))] | length'
And got 2 as the output.
Upon closer examination, when scanning snyk/cli/test/fixtures/iac/cloudformation/aurora-valid.yml:
snyk iac test --json test/fixtures/iac/cloudformation/aurora-valid.yml | jq '[.infrastructureAsCodeIssues[] | .type]'
I got the following:
null,
null,
"terraform",
null,
null,
null,
null,
null,
"terraform",
null
]
| "policyEngineType": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
this does not exist in all policies either. but like before, it's ok as it's not required. 👍
There was a problem hiding this comment.
Just ran the following command in snyk/cli/test/fixtures/iac/cloudformation:
snyk iac test ./test/fixtures/iac/cloudformation --json | jq '[.[].infrastructureAsCodeIssues[] | keys | select(contains(["policyEngineType"]))] | length'
And got 2 as the output.
Upon closer examination, when scanning snyk/cli/test/fixtures/iac/cloudformation/aurora-valid.yml:
snyk iac test --json test/fixtures/iac/cloudformation/aurora-valid.yml | jq '[.infrastructureAsCodeIssues[] | .policyEngineType]'
I got the following:
null,
null,
"opa",
null,
null,
null,
null,
null,
"opa",
null
]
| "name": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
can't find this in any of my example runs either
| "from": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, |
There was a problem hiding this comment.
can't find this in any of my example runs either
| "ignoreSettings": { | ||
| "type": "object", | ||
| "properties": { | ||
| "adminOnly": { | ||
| "type": "boolean" | ||
| }, | ||
| "reasonRequired": { | ||
| "type": "boolean" | ||
| }, | ||
| "disregardFilesystemIgnores": { | ||
| "type": "boolean" | ||
| } | ||
| }, |
There was a problem hiding this comment.
this ignoreSettings object is null on my local example run.
There was a problem hiding this comment.
On a separate note, it looks like we also hard-code policy to be an empty string, which is not changed by any of the flows we currently have, including Share Results. I added a maxLength: 0 requirement for it.
| @@ -0,0 +1,200 @@ | |||
| import * as fs from 'fs'; | |||
| import * as pathLib from 'path'; | |||
| import { matchers } from 'jest-json-schema'; | |||
There was a problem hiding this comment.
oooh alright. so in the end you didn't use ajv to compile/validate the json when we build it, but instead you used this package which imports ajv internally, and you run it in the test. nice
There was a problem hiding this comment.
Going through this article, it became apparent that in order to integrate AJV with Jest, some global test suite configuration would need to be updated, which included expanding expect to include the needed custom matchers, and edit the global types for Jest.
These changes were all provided by this third-party package, jest-json-schema
ofekatr
force-pushed
the
chore/add-tests-for-test-json-output
branch
from
0d999b6
to
88d2762
Compare
ofekatr
force-pushed
the
chore/add-tests-for-test-json-output
branch
from
88d2762
to
f66ee0d
Compare
At the moment we avoided creating any acceptance tests for the experimental test flow. We do however validate the IaC issue properties and values in the JSON output format in test/jest/unit/lib/iac/test/v2/json.spec.ts. |
What does this PR do?
snyk iac testcommand.Where should the reviewer start?
Any background context you want to provide?
avjwas used, following this suggestions.Notes for the reviewer