fixer: Prep for 1.0 tests (#1319)

* fixer: Prep for 1.0 tests Signed-off-by: Charlie Egan <[email protected]> * fixer/fmt: Add tests for other rego versions Signed-off-by: Charlie Egan <[email protected]> * fixer: Update fixer e2e test Signed-off-by: Charlie Egan <[email protected]> * Correct formatting of launch.json * fixer: Return formatting to Fixer test Signed-off-by: Charlie Egan <[email protected]> --------- Signed-off-by: Charlie Egan <[email protected]>
StyraInc · Jan 8, 2025 · 83b8f33 · 83b8f33
1 parent dd51d47
commit 83b8f33
Show file tree

Hide file tree

Showing 7 changed files with 165 additions and 72 deletions.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -26,4 +26,4 @@
       ]
     }
   ]
-}
+}
diff --git a/e2e/cli_test.go b/e2e/cli_test.go
@@ -288,8 +288,6 @@ func TestLintV0WithRegoV1ImportViolations(t *testing.T) {
 		t.Errorf("expected stderr %q, got %q", exp, act)
 	}
 
-	//fmt.Println(stdout.String())
-
 	var rep report.Report
 	if err = json.Unmarshal(stdout.Bytes(), &rep); err != nil {
 		t.Fatalf("expected JSON response, got %v", stdout.String())
@@ -836,11 +834,17 @@ func TestFix(t *testing.T) {
 	td := t.TempDir()
 
 	initialState := map[string]string{
-		".regal/config.yaml": "", // needed to find the root in the right place
+		".regal/config.yaml": `
+project:
+  rego-version: 1
+  roots:
+    - path: v0
+      rego-version: 0
+    - path: v1
+      rego-version: 0
+`,
 		"foo/main.rego": `package wow
 
-import rego.v1
-
 #comment
 
 allow if {
@@ -849,28 +853,34 @@ allow if {
 `,
 		"foo/main_test.rego": `package wow_test
 
-test_allow {
+test_allow if {
 	true
 }
 `,
 		"foo/foo.rego": `package foo
 
-import rego.v1
-
 # present and correct
 
 allow if {
 	input.admin
 }
 `,
 		"bar/main.rego": `package wow["foo-bar"].baz
-
-import rego.v1
 `,
 		"bar/main_test.rego": `package wow["foo-bar"].baz_test
-test_allow {
+test_allow if {
 	true
 }
+`,
+		"v0/main.rego": `package v0
+
+#comment
+allow { true }
+`,
+		"v1/main.rego": `package v1
+
+#comment
+allow if { true }
 `,
 		"unrelated.txt": `foobar`,
 	}
@@ -880,29 +890,46 @@ test_allow {
 	}
 
 	// --force is required to make the changes when there is no git repo
-	err := regal(&stdout, &stderr)("fix", "--force", filepath.Join(td, "foo"), filepath.Join(td, "bar"))
+	err := regal(&stdout, &stderr)(
+		"fix",
+		"--force",
+		filepath.Join(td, "foo"),
+		filepath.Join(td, "bar"),
+		filepath.Join(td, "v0"),
+		filepath.Join(td, "v1"),
+	)
 
 	// 0 exit status is expected as all violations should have been fixed
 	expectExitCode(t, err, 0, &stdout, &stderr)
 
-	exp := fmt.Sprintf(`8 fixes applied:
-In project root: %s
+	exp := fmt.Sprintf(`12 fixes applied:
+In project root: %[1]s
 bar/main.rego -> wow/foo-bar/baz/main.rego:
 - directory-package-mismatch
 
 bar/main_test.rego -> wow/foo-bar/baz/main_test.rego:
 - directory-package-mismatch
-- use-rego-v1
+- opa-fmt
 
 foo/main.rego -> wow/main.rego:
 - directory-package-mismatch
-- use-rego-v1
+- opa-fmt
 - no-whitespace-comment
 
 foo/main_test.rego -> wow/main_test.rego:
 - directory-package-mismatch
-- use-rego-v1
+- opa-fmt
+
+
+In project root: %[1]s/v0
+main.rego:
+- opa-fmt
+- no-whitespace-comment
 
+In project root: %[1]s/v1
+main.rego:
+- opa-fmt
+- no-whitespace-comment
 `, td)
 
 	if act := stdout.String(); exp != act {
@@ -916,37 +943,39 @@ foo/main_test.rego -> wow/main_test.rego:
 	expectedState := map[string]string{
 		"foo/foo.rego": `package foo
 
-import rego.v1
-
 # present and correct
 
 allow if {
 	input.admin
 }
 `,
 		"wow/foo-bar/baz/main.rego": `package wow["foo-bar"].baz
-
-import rego.v1
 `,
 		"wow/foo-bar/baz/main_test.rego": `package wow["foo-bar"].baz_test
 
-import rego.v1
-
 test_allow := true
 `,
 		"wow/main.rego": `package wow
 
-import rego.v1
-
 # comment
 
 allow := true
 `,
 		"wow/main_test.rego": `package wow_test
 
+test_allow := true
+`,
+		"v0/main.rego": `package v0
+
 import rego.v1
 
-test_allow := true
+# comment
+allow := true
+`,
+		"v1/main.rego": `package v1
+
+# comment
+allow := true
 `,
 		"unrelated.txt": `foobar`,
 	}

diff --git a/internal/lsp/server.go b/internal/lsp/server.go
@@ -2165,14 +2165,7 @@ func (l *LanguageServer) handleTextDocumentFormatting(
 
 		f := fixer.NewFixer()
 		f.RegisterFixes(fixes.NewDefaultFormatterFixes()...)
-		f.RegisterMandatoryFixes(
-			&fixes.Fmt{
-				NameOverride: "use-rego-v1",
-				OPAFmtOpts: format.Opts{
-					RegoVersion: ast.RegoV0CompatV1,
-				},
-			},
-		)
+		f.RegisterMandatoryFixes([]fixes.Fix{&fixes.Fmt{}}...)
 
 		if roots, err := config.GetPotentialRoots(
 			l.workspacePath(),

diff --git a/pkg/fixer/fixer_test.go b/pkg/fixer/fixer_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/open-policy-agent/opa/v1/ast"
 	"github.com/open-policy-agent/opa/v1/format"
 
+	"github.com/styrainc/regal/pkg/config"
 	"github.com/styrainc/regal/pkg/fixer/fileprovider"
 	"github.com/styrainc/regal/pkg/fixer/fixes"
 	"github.com/styrainc/regal/pkg/linter"
@@ -20,10 +21,9 @@ func TestFixer(t *testing.T) {
 	policies := map[string][]byte{
 		"test/main.rego": []byte(`package test
 
-allow {
+allow if {
 true #no space
 }
-
 deny = true
 `),
 	}
@@ -48,14 +48,12 @@ deny = true
 	}
 
 	expectedFileFixedViolations := map[string][]string{
-		// use-assignment-operator is not expected since use-rego-v1 also addresses this in this example
-		"test/main.rego": {"no-whitespace-comment", "use-rego-v1"},
+		// use-assigment-operator is correct in formatting so does not appear.
+		"test/main.rego": {"no-whitespace-comment", "opa-fmt"},
 	}
 	expectedFileContents := map[string][]byte{
 		"test/main.rego": []byte(`package test
 
-import rego.v1
-
 allow := true
 
 # no space
@@ -65,7 +63,7 @@ deny := true
 	}
 
 	if got, exp := fixReport.TotalFixes(), uint(2); got != exp {
-		t.Fatalf("expected %d fixed files, got %d", exp, got)
+		t.Fatalf("expected a total of %d fixes, got %d", exp, got)
 	}
 
 	fpFiles, err := memfp.List()
@@ -95,19 +93,21 @@ deny := true
 		// check that the fixed violations are correct
 		fxs := fixReport.FixesForFile(file)
 
+		var fixes []string
+		for _, fx := range fxs {
+			fixes = append(fixes, fx.Title)
+		}
+
 		expectedFixes, ok := expectedFileFixedViolations[file]
 		if !ok {
-			t.Fatalf("unexpected file waas fixed %s", file)
+			t.Fatalf("unexpected file was fixed %s", file)
 		}
 
-		if len(fxs) != len(expectedFixes) {
-			t.Fatalf("unexpected number of fixes for %s:\ngot: %v\nexpected: %v", file, fxs, expectedFixes)
-		}
+		slices.Sort(expectedFixes)
+		slices.Sort(fixes)
 
-		for _, fx := range fxs {
-			if !slices.Contains(expectedFixes, fx.Title) {
-				t.Fatalf("expected fixes to contain %s:\ngot: %v", fx.Title, expectedFixes)
-			}
+		if !slices.Equal(expectedFixes, fixes) {
+			t.Fatalf("unexpected fixes for %s:\ngot: %v\nexpected: %v", file, fixes, expectedFixes)
 		}
 	}
 }
@@ -135,7 +135,12 @@ deny = true
 
 	l := linter.NewLinter().
 		WithEnableAll(true).
-		WithInputModules(&input)
+		WithInputModules(&input).
+		WithUserConfig(config.Config{
+			Capabilities: &config.Capabilities{
+				Features: []string{"rego_v1_import"},
+			},
+		})
 
 	f := NewFixer()
 	// No fixes are registered here, we are only testing the functionality of
@@ -173,7 +178,7 @@ deny := true
 	}
 
 	if got, exp := fixReport.TotalFixes(), uint(1); got != exp {
-		t.Fatalf("expected %d fixed files, got %d", exp, got)
+		t.Fatalf("expected %d fixes, got %d", exp, got)
 	}
 
 	fpFiles, err := memfp.List()
@@ -200,22 +205,24 @@ deny := true
 				string(expectedContent))
 		}
 
-		// check that the fixed violations are correct
 		fxs := fixReport.FixesForFile(file)
 
+		// check that the fixed violations are correct
+		var fixes []string
+		for _, fx := range fxs {
+			fixes = append(fixes, fx.Title)
+		}
+
 		expectedFixes, ok := expectedFileFixedViolations[file]
 		if !ok {
-			t.Fatalf("unexpected file waas fixed %s", file)
+			t.Fatalf("unexpected file was fixed %s", file)
 		}
 
-		if len(fxs) != len(expectedFixes) {
-			t.Fatalf("unexpected number of fixes for %s:\ngot: %v\nexpected: %v", file, fxs, expectedFixes)
-		}
+		slices.Sort(expectedFixes)
+		slices.Sort(fixes)
 
-		for _, fx := range fxs {
-			if !slices.Contains(expectedFixes, fx.Title) {
-				t.Fatalf("expected fixes to contain %s:\ngot: %v", fx.Title, expectedFixes)
-			}
+		if !slices.Equal(expectedFixes, fixes) {
+			t.Fatalf("unexpected fixes for %s:\ngot: %v\nexpected: %v", file, fixes, expectedFixes)
 		}
 	}
 }
diff --git a/pkg/fixer/fixes/fixes.go b/pkg/fixer/fixes/fixes.go
@@ -2,7 +2,6 @@ package fixes
 
 import (
 	"github.com/open-policy-agent/opa/v1/ast"
-	"github.com/open-policy-agent/opa/v1/format"
 
 	"github.com/styrainc/regal/internal/lsp/clients"
 	"github.com/styrainc/regal/pkg/config"
@@ -14,10 +13,9 @@ func NewDefaultFixes() []Fix {
 	return []Fix{
 		&Fmt{},
 		&Fmt{
+			// this effectively maps the fix for violations from the
+			// use-rego-v1 rule to just format the file.
 			NameOverride: "use-rego-v1",
-			OPAFmtOpts: format.Opts{
-				RegoVersion: ast.RegoV0CompatV1,
-			},
 		},
 		&UseAssignmentOperator{},
 		&NoWhitespaceComment{},
@@ -29,12 +27,7 @@ func NewDefaultFixes() []Fix {
 // Notably, this does not include fixers that move files around.
 func NewDefaultFormatterFixes() []Fix {
 	return []Fix{
-		&Fmt{
-			NameOverride: "use-rego-v1",
-			OPAFmtOpts: format.Opts{
-				RegoVersion: ast.RegoV0CompatV1,
-			},
-		},
+		&Fmt{},
 		&UseAssignmentOperator{},
 		&NoWhitespaceComment{},
 	}
-Original file line number
+Diff line change
@@ Expand Up / @@ -26,4 +26,4 @@ @@
           ]
         }
       ]
-    }
+    }