Use Testify in all Go-based Secure project tests
Problem to solve
Unclear test output
Secure tests currently use either reflect.DeepEqual or an equality operator to determine whether the actual
result matches the expected
result. Both of these comparisons will yield a wall of text when the comparison fails, for example:
--- FAIL: TestIssue (0.00s)
--- FAIL: TestIssue/MarshalJSON (0.00s)
--- FAIL: TestIssue/MarshalJSON/SAST (0.00s)
issue_test.go:414: Wrong result. Expected:
"{\n \"id\":
\"1e029c1314252858f37ab31b517c4bd5e99c22eb91b9331e0ee3b65cf3b0a5853\",\n
\"category\": \"sast\",\n \"name\": \"Possible command injection\",\n \"message\":
\"Possible command injection\",\n \"description\": \"The ciphertext produced is
susceptible to alteration by an adversary.\",\n \"cve\":
\"09abf078636daa3bc6a3d49fba232774a5d9b7c454e1003e6ab12cd56938296a\",\n
\"severity\": \"Medium\",\n \"confidence\": \"High\",\n \"solution\": \"Use the
system(command, parameters) method which passes command line parameters safely.\",\n
\"scanner\": {\n \"id\": \"brakeman\",\n \"name\": \"Brakeman\"\n },\n \"location\":
{\n \"file\": \"app/controllers/application_controller.rb\",\n \"start_line\":
831,\n \"end_line\": 832\n },\n \"identifiers\": [\n {\n \"type\": \"cve\",\n
\"name\": \"CVE-2018-1234\",\n \"value\": \"CVE-2018-1234\",\n \"url\":
\"https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-1234\"\n }\n ],\n
\"links\": [\n {\n \"name\": \"Awesome-security blog post\",\n \"url\":
\"https://example.com/blog-post\"\n },\n {\n \"url\":
\"https://example.com/another-blog-post\"\n }\n ],\n \"scan\": {\n \"scanner\": {\n
\"id\": \"brakeman\",\n \"name\": \"Brakeman\"\n },\n \"type\": \"sast\"\n }\n}"
But got:
"{\n \"id\":
\"e029c1314252858f37ab31b517c4bd5e99c22eb91b9331e0ee3b65cf3b0a5853\",\n
\"category\": \"sast\",\n \"name\": \"Possible command injection\",\n \"message\":
\"Possible command injection\",\n \"description\": \"The ciphertext produced is
susceptible to alteration by an adversary.\",\n \"cve\":
\"09abf078636daa3bc6a3d49fba232774a5d9b7c454e1003e6ab12cd56938296a\",\n
\"severity\": \"Medium\",\n \"confidence\": \"High\",\n \"solution\": \"Use the
system(command, parameters) method which passes command line parameters safely.\",\n
\"scanner\": {\n \"id\": \"brakeman\",\n \"name\": \"Brakeman\"\n },\n \"location\":
{\n \"file\": \"app/controllers/application_controller.rb\",\n \"start_line\":
831,\n \"end_line\": 832\n },\n \"identifiers\": [\n {\n \"type\": \"cve\",\n
\"name\": \"CVE-2018-1234\",\n \"value\": \"CVE-2018-1234\",\n \"url\":
\"https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-1234\"\n }\n ],\n
\"links\": [\n {\n \"name\": \"Awesome-security blog post\",\n \"url\":
\"https://example.com/blog-post\"\n },\n {\n \"url\":
\"https://example.com/another-blog-post\"\n }\n ],\n \"scan\": {\n \"scanner\": {\n
\"id\": \"brakeman\",\n \"name\": \"Brakeman\"\n },\n \"type\": \"sast\"\n }\n}"
FAIL
exit status 1
FAIL gitlab.com/gitlab-org/security-products/analyzers/common/v2/issue 0.259s
This makes it really difficult to understand what part of the comparison failed. If we instead switch to using assert.JSONeq
, then it becomes much easier to understand the reason for the failure:
--- FAIL: TestIssue (0.00s)
--- FAIL: TestIssue/MarshalJSON (0.00s)
--- FAIL: TestIssue/MarshalJSON/SAST (0.00s)
issue_test.go:411:
Error Trace: issue_test.go:411
Error: Not equal:
Diff:
--- Expected
+++ Actual
@@ -5,3 +5,3 @@
- (string) (len=2) "id": (string) (len=65) "1e029c1314252858f37ab31b517c4bd5e99c22eb91b9331e0ee3b65cf3b0a5853",
+ (string) (len=2) "id": (string) (len=64) "e029c1314252858f37ab31b517c4bd5e99c22eb91b9331e0ee3b65cf3b0a5853",
(string) (len=11) "identifiers": ([]interface {}) (len=1) {
Test: TestIssue/MarshalJSON/SAST
Reduced boilerplate testing code
In addition to making test failure output clearer, using assertions from the testify package will also reduce boilerplate testing code.
For example, instead of constantly inserting checks such as the following:
if !reflect.DeepEqual(want, got) {
t.Errorf("Wrong result. Expected:\n%#v\nbut got:\n%#v", want, got)
}
We can simply insert an assertion:
assert.Equal(t, want, got)
Simplified custom matchers
This change will allow us to remove some custom JSON Matching code which can be replaced by using testify.JSONeq
Removal of custom sorting functions
We'll also be able to use assert.ElementsMatch()
to perform order-agnostic element matching, allowing us to remove a large amount of sorting code which is currently used to enforce consistency in some of our tests.
Reduction of incorrectly written tests
Finally, using assertions will reduce the possibility of errors such as the following from occurring and by using github.com/stretchr/testify/require
, we can ensure that a failure will halt tests immediately, instead of needing to remember to use t.Fatalf
Intended users
Proposal
Update the projects to use the testify.Assert package.
We'll focus on Dependency Scanning first, then open follow-up issues for other groups if they are interested in following the same approach.
In particular, the unit tests for Gemnasium's dependency file parsers can be simplified, and a lot more readable. See gitlab-org/security-products/analyzers/gemnasium!87 (diffs, comment 388789397)
- Dependency Scanning
Other groups
- Container Security
- Static Analysis (already using it in some projects)
What does success look like, and how can we measure that?
Tests in Secure projects no longer use reflect.DeepEqual
or plain equality operators - every comparison in a test uses an assertion from testify
Is this a cross-stage feature?
Yes, this will affect all secure projects