All tests should use `TestCheckFunc` helpers #195

detro · 2022-04-28T16:03:20Z

Context: https://github.com/hashicorp/terraform-providers-devex-internal/issues/94
Related RFC: https://docs.google.com/document/d/1_zUbW-PYvsr0IKoO22GhANcmiaL9OxAwYCLkHomL2-0/edit#

PLEASE NOTE: This will fail testing until hashicorp/terraform-plugin-sdk#950 is merged and a new release (v2.15.0) of terraform-plugin-sdk is cut.

bendbennett

LGTM

bflad

Ah looks so much better, left a few small things to consider, but otherwise looks good to me. 🚀

bflad · 2022-05-05T14:24:35Z

internal/provider/resource_cert_request_test.go

+					r.TestCheckResourceAttrWith("tls_cert_request.test1", "cert_request_pem", func(value string) error {
+						block, _ := pem.Decode([]byte(value))
+						csr, err := x509.ParseCertificateRequest(block.Bytes)
+						if err != nil {
+							return fmt.Errorf("error parsing CSR: %s", err)
+						}
+						if expected, got := "2", csr.Subject.SerialNumber; got != expected {


Nit: Since there are repeating checks for *x509.CertificateRequest contents, would it be easier to introduce a helper function in this provider codebase? e.g.

func testCheckPEMCertificateRequest(expected *x509.CertificateRequest) resource.TestCheckResourceAttrWithFunc { return func(value string) error { // decode and parse // equality logic (bummer there's not an Equal() method on it) } }

Then the test can remain declarative:

r.TestCheckResourceAttrWith("tls_cert_request.test1", "cert_request_pem", testCheckPEMCertificateRequest(&x509.CertificateRequest{ Subject: x509.Subject{ CommonName: "example.com", SerialNumber: 2, // ... etc }, // ... etc }),

(or the test helper can wrap away the TestCheckResourceAttrWith() completely, e.g.)

func testCheckPEMCertificateRequest(name string, key string, expected *x509.CertificateRequest) resource.TestCheckFunc { return func(s *terraform.State) error { return resource.TestCheckResourceAttrWith(name, key, func(value string) error { // decode and parse // equality logic (bummer there's not an Equal() method on it) })(s) } }

Will give this a go.

bflad · 2022-05-05T14:25:50Z

internal/provider/resource_locally_signed_cert_test.go

+					r.TestCheckResourceAttrWith("tls_locally_signed_cert.test", "cert_pem", func(value string) error {
+						block, _ := pem.Decode([]byte(value))
+						cert, err := x509.ParseCertificate(block.Bytes)
+						if err != nil {
+							return fmt.Errorf("error parsing cert: %s", err)
+						}
+						if expected, got := "2", cert.Subject.SerialNumber; got != expected {
+							return fmt.Errorf("incorrect subject serial number: expected %v, got %v", expected, got)
+						}


Nit: Similar question for creating a provider codebase test helper for x509.Certificate

bflad · 2022-05-05T14:28:06Z

internal/provider/resource_private_key_test.go

@@ -18,119 +17,38 @@ func TestPrivateKeyRSA(t *testing.T) {
 					resource "tls_private_key" "test" {
 						algorithm = "RSA"
 					}
-					output "private_key_pem" {


bflad · 2022-05-05T14:30:36Z

internal/provider/resource_private_key_test.go

-					return nil
-				},
+				Check: r.ComposeAggregateTestCheckFunc(
+					r.TestMatchResourceAttr("tls_private_key.test", "private_key_pem", regexp.MustCompile(`^-----BEGIN EC PRIVATE KEY----(.|\s)+-----END EC PRIVATE KEY-----`)),


Nit: Given #197, should these regular expressions explicitly check the end of the strings as well for newlines, etc. to prevent regressions?

…dev_overrides` in `.terraformrc`

…h a regexp that considers both beginning and end of the string

…testing of PEM certificates more declarative This is still a work in progress: in this first iteration we have moved `tls_cert_request` resouce to use it.

bflad

Looking real nice with that TestCheckResourceAttrWith() -- just some small things for consideration, otherwise looks good to me 🚀

bflad · 2022-05-09T13:57:00Z

internal/provider/test_check_func_test.go

+)
+
+func testCheckPEMCertificateFormat(name, key string, expected PEMPreamble) r.TestCheckFunc {
+	return r.TestMatchResourceAttr(name, key, regexp.MustCompile(fmt.Sprintf(`^-----BEGIN %[1]s----(.|\s)+-----END %[1]s-----\n$`, expected)))


Two questions for you:

Is this missing a 5th dash after the opening preamble?

Should it check explicitly for a newline after the opening preamble and before the closing preamble?

good catches: I have in the end come up with a dedicated PEM format checker and will make sure it cover those.

👍

bflad · 2022-05-09T14:00:45Z

internal/provider/resource_locally_signed_cert_test.go

-					}
+				Config: locallySignedCertConfig(1, 0),
+				Check: r.ComposeAggregateTestCheckFunc(
+					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`^-----BEGIN CERTIFICATE----(.|\s)+-----END CERTIFICATE-----\n$`)),


Should this be using testCheckPEMCertificateFormat()?

bflad · 2022-05-09T14:01:09Z

internal/provider/resource_locally_signed_cert_test.go

-					}
+				Config: locallySignedCertConfigWithDeprecatedKeyAlgorithm(1, 0),
+				Check: r.ComposeAggregateTestCheckFunc(
+					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`^-----BEGIN CERTIFICATE----(.|\s)+-----END CERTIFICATE-----\n$`)),


Similarly regarding testCheckPEMCertificateFormat()

bflad · 2022-05-09T14:01:23Z

internal/provider/resource_locally_signed_cert_test.go

@@ -509,7 +454,7 @@ func TestAccResourceLocallySignedCert_FromED25519PrivateKeyResource(t *testing.T
 				`,
 				Check: r.ComposeTestCheckFunc(
 					r.TestCheckResourceAttr("tls_locally_signed_cert.test", "ca_key_algorithm", "ED25519"),
-					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`-----BEGIN CERTIFICATE-----((.|\n)+?)-----END CERTIFICATE-----`)),
+					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`-----BEGIN CERTIFICATE-----(.|\s)+-----END CERTIFICATE-----\n$`)),


bflad · 2022-05-09T14:01:43Z

internal/provider/resource_locally_signed_cert_test.go

@@ -561,7 +504,7 @@ func TestAccResourceLocallySignedCert_FromECDSAPrivateKeyResource(t *testing.T)
 				`,
 				Check: r.ComposeTestCheckFunc(
 					r.TestCheckResourceAttr("tls_locally_signed_cert.test", "ca_key_algorithm", "ECDSA"),
-					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`-----BEGIN CERTIFICATE-----((.|\n)+?)-----END CERTIFICATE-----`)),
+					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`^-----BEGIN CERTIFICATE----(.|\s)+-----END CERTIFICATE-----\n$`)),


bflad · 2022-05-09T14:01:50Z

internal/provider/resource_locally_signed_cert_test.go

@@ -613,7 +554,7 @@ func TestAccResourceLocallySignedCert_FromRSAPrivateKeyResource(t *testing.T) {
 				`,
 				Check: r.ComposeTestCheckFunc(
 					r.TestCheckResourceAttr("tls_locally_signed_cert.test", "ca_key_algorithm", "RSA"),
-					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`-----BEGIN CERTIFICATE-----((.|\n)+?)-----END CERTIFICATE-----`)),
+					r.TestMatchResourceAttr("tls_locally_signed_cert.test", "cert_pem", regexp.MustCompile(`^-----BEGIN CERTIFICATE----(.|\s)+-----END CERTIFICATE-----\n$`)),


bflad · 2022-05-09T14:02:21Z

internal/provider/resource_private_key_test.go

-					return nil
-				},
+				Check: r.ComposeAggregateTestCheckFunc(
+					r.TestMatchResourceAttr("tls_private_key.test", "private_key_pem", regexp.MustCompile(`^-----BEGIN RSA PRIVATE KEY----(.|\s)+-----END RSA PRIVATE KEY-----\n$`)),


I'll just 😉 once for this file. 😄

bflad · 2022-05-09T14:02:58Z

internal/provider/resource_self_signed_cert_test.go

-					}
+				Config: selfSignedCertConfig(1, 0),
+				Check: r.ComposeAggregateTestCheckFunc(
+					r.TestMatchResourceAttr("tls_self_signed_cert.test1", "cert_pem", regexp.MustCompile(`^-----BEGIN CERTIFICATE----(.|\s)+-----END CERTIFICATE-----\n$`)),


Similarly for this file regarding testCheckPEMCertificateFormat()

…linter false alerts

bflad

🚀

* Revamp all tests to use TestCheckFunc helpers * .go-version: 1.17.8 -> 1.17.9 * Updating doc to refer to the official page that explains how to use `dev_overrides` in `.terraformrc` * lint * Altering tests that check format of PEM (keys and certs) to match with a regexp that considers both beginning and end of the string * Adding utilities built on top of `TestCheckResourceAttrWith` to make testing of PEM certificates more declarative This is still a work in progress: in this first iteration we have moved `tls_cert_request` resouce to use it. * Turn PEM validation declarative for all types of PEM we produce * Appease linter (unparam) for implementations of `TestCheckFunc` * Use `//nolint:unparam` instead of `exclude-rules:` for very specific linter false alerts

detro requested a review from a team as a code owner April 28, 2022 16:03

github-actions bot added dependencies size/XL labels Apr 28, 2022

detro added this to the next.minor milestone Apr 28, 2022

detro force-pushed the detro/all_tests_should_TestCheckFunc branch from 0631798 to 0bd7209 Compare May 5, 2022 10:58

github-actions bot removed the dependencies label May 5, 2022

bendbennett approved these changes May 5, 2022

View reviewed changes

bflad approved these changes May 5, 2022

View reviewed changes

Ivan De Marino added 5 commits May 5, 2022 17:08

Revamp all tests to use TestCheckFunc helpers

58c923b

.go-version: 1.17.8 -> 1.17.9

7b95980

Updating doc to refer to the official page that explains how to use `…

3905fe8

…dev_overrides` in `.terraformrc`

lint

258a2a3

Altering tests that check format of PEM (keys and certs) to match wit…

d4bca12

…h a regexp that considers both beginning and end of the string

detro force-pushed the detro/all_tests_should_TestCheckFunc branch from 200d16b to d4bca12 Compare May 5, 2022 16:42

Adding utilities built on top of TestCheckResourceAttrWith to make …

6396e82

…testing of PEM certificates more declarative This is still a work in progress: in this first iteration we have moved `tls_cert_request` resouce to use it.

bflad approved these changes May 9, 2022

View reviewed changes

Ivan De Marino added 3 commits May 9, 2022 18:04

Turn PEM validation declarative for all types of PEM we produce

0b933c1

Appease linter (unparam) for implementations of TestCheckFunc

566ac29

Use //nolint:unparam instead of exclude-rules: for very specific …

9a48156

…linter false alerts

bflad approved these changes May 9, 2022

View reviewed changes

detro merged commit e99e692 into main May 9, 2022

detro deleted the detro/all_tests_should_TestCheckFunc branch May 9, 2022 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All tests should use `TestCheckFunc` helpers #195

All tests should use `TestCheckFunc` helpers #195

detro commented Apr 28, 2022 •

edited

bendbennett left a comment

bflad left a comment

bflad May 5, 2022

bflad May 5, 2022

detro May 5, 2022

bflad May 5, 2022

bflad May 5, 2022

bflad May 5, 2022

bflad left a comment

bflad May 9, 2022

detro May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad May 9, 2022

bflad left a comment

All tests should use TestCheckFunc helpers #195

All tests should use TestCheckFunc helpers #195

Conversation

detro commented Apr 28, 2022 • edited

bendbennett left a comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

All tests should use `TestCheckFunc` helpers #195

All tests should use `TestCheckFunc` helpers #195

detro commented Apr 28, 2022 •

edited