From 96e4804600a7e5f2abe47eafc488254b2037252e Mon Sep 17 00:00:00 2001
From: Charlie Egan <charlie@styra.com>
Date: Thu, 31 Oct 2024 09:20:21 +0000
Subject: [PATCH] rule: Disable leaked_internal_reference for test files
 (#1228)

* rule: Disable leaked_internal_reference for test files

Option added to turn back on if needed

Signed-off-by: Charlie Egan <charlie@styra.com>

* Explain the rule is of by default for tests

Signed-off-by: Charlie Egan <charlie@styra.com>

---------

Signed-off-by: Charlie Egan <charlie@styra.com>
---
 .../leaked_internal_reference.rego            | 22 +++++++++++++++++
 .../leaked_internal_reference_test.rego       | 24 +++++++++++++++++++
 docs/rules/bugs/leaked-internal-reference.md  | 10 ++++++++
 3 files changed, 56 insertions(+)

diff --git a/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference.rego b/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference.rego
index ed17c20e..c5d19df3 100644
--- a/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference.rego
+++ b/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference.rego
@@ -5,9 +5,12 @@ package regal.rules.bugs["leaked-internal-reference"]
 import rego.v1
 
 import data.regal.ast
+import data.regal.config
 import data.regal.result
 
 report contains violation if {
+	_enabled(_valid_test_file_name(input.regal.file.name), _enable_for_test_files)
+
 	ref := ast.found.refs[_][_]
 
 	contains(ast.ref_to_string(ref.value), "._")
@@ -16,9 +19,28 @@ report contains violation if {
 }
 
 report contains violation if {
+	_enabled(_valid_test_file_name(input.regal.file.name), _enable_for_test_files)
+
 	some imported in input.imports
 
 	contains(ast.ref_to_string(imported.path.value), "._")
 
 	violation := result.fail(rego.metadata.chain(), result.ranged_from_ref(imported.path.value))
 }
+
+default _enabled(_, _) := true
+
+_enabled(true, false) := false
+
+default _valid_test_file_name(_) := false
+
+_valid_test_file_name(filename) if endswith(filename, "_test.rego")
+
+# Styra DAS convention considered OK
+_valid_test_file_name("test.rego")
+
+_cfg := config.for_rule("bugs", "leaked-internal-reference")
+
+default _enable_for_test_files := false
+
+_enable_for_test_files := object.get(_cfg, "include-test-files", false)
diff --git a/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference_test.rego b/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference_test.rego
index d6e17209..8dce576e 100644
--- a/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference_test.rego
+++ b/bundle/regal/rules/bugs/leaked-internal-reference/leaked_internal_reference_test.rego
@@ -68,6 +68,30 @@ test_fail_leaked_internal_reference_in_nested_comprehension if {
 	})
 }
 
+test_ignore_test_file_by_default if {
+	r := rule.report with input as ast.with_rego_v1(`foo := data.bar._wow`)
+		with input.regal.file.name as "p_test.rego"
+
+	r == set()
+}
+
+test_ignore_test_file_can_be_disabled if {
+	r := rule.report with input as ast.with_rego_v1(`foo := data.bar._wow`)
+		with input.regal.file.name as "p_test.rego"
+		with config.for_rule as {"include-test-files": true}
+
+	r == expected_with_location({
+		"file": "p_test.rego",
+		"col": 8,
+		"row": 5,
+		"end": {
+			"col": 21,
+			"row": 5,
+		},
+		"text": "foo := data.bar._wow",
+	})
+}
+
 expected := {
 	"category": "bugs",
 	"description": "Outside reference to internal rule or function",
diff --git a/docs/rules/bugs/leaked-internal-reference.md b/docs/rules/bugs/leaked-internal-reference.md
index f9e3a461..cb9a3451 100644
--- a/docs/rules/bugs/leaked-internal-reference.md
+++ b/docs/rules/bugs/leaked-internal-reference.md
@@ -5,6 +5,7 @@
 **Category**: Bugs
 
 **Avoid**
+
 ```rego
 package policy
 
@@ -44,6 +45,14 @@ Do note that if you disagree with this rule, you don't need to disable it unless
 something else. If you don't use underscore prefixes, nothing will be reported by this rule anyway. It does however
 mean that the benefits listed above won't apply to your project.
 
+## Exceptions
+
+This rule is not enabled by default for test files. In tests, it can be useful
+to reference internal rules and functions to achieve good test coverage, which
+would be a violation of this rule. If you want to run this rule for tests
+too, you can set `include-test-files: true` in the configuration for this rule
+in your Regal config file.
+
 ## Configuration Options
 
 This linter rule provides the following configuration options:
@@ -54,6 +63,7 @@ rules:
     leaked-internal-reference:
       # one of "error", "warning", "ignore"
       level: error
+      include-test-files: false # default is false
 ```
 
 ## Related Resources