Skip to content

Commit c24c127

Browse files
committed
Update Fix to Properly Locate Error and Address Test PR Feedback
Signed-off-by: Sean Ledford <s_ledford@apple.com>
1 parent 77cc723 commit c24c127

2 files changed

Lines changed: 93 additions & 24 deletions

File tree

pkg/fixer/fixes/preferequalscomparison.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@ func (p *PreferEqualsComparison) Fix(fc *FixCandidate, opts *RuntimeOptions) ([]
2525
continue
2626
}
2727

28-
// unexpected character at location column, skipping
29-
if line[loc.Column-1] != '=' {
28+
eqIndex := strings.Index(line, "=")
29+
30+
// unification operator not found, skipping
31+
if eqIndex == -1 {
3032
continue
3133
}
3234

33-
lines[loc.Row-1] = line[0:loc.Column] + "=" + line[loc.Column:]
35+
lines[loc.Row-1] = line[0:eqIndex] + "=" + line[eqIndex:]
3436
fixed = true
3537
}
3638

pkg/fixer/fixes/preferequalscomparison_test.go

Lines changed: 88 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package fixes
33
import (
44
"testing"
55

6+
"github.com/google/go-cmp/cmp"
7+
68
"github.com/open-policy-agent/regal/pkg/report"
79
)
810

@@ -22,40 +24,105 @@ func TestPreferEqualsComparison(t *testing.T) {
2224
runtimeOptions: &RuntimeOptions{},
2325
},
2426
"no change because no location": {
25-
fc: &FixCandidate{Filename: "test.rego", Contents: "package test\n\nallow = true\n"},
26-
contentAfterFix: "package test\n\nallow = true\n",
27-
fixExpected: false,
28-
runtimeOptions: &RuntimeOptions{},
27+
fc: &FixCandidate{
28+
Filename: "test.rego",
29+
Contents: `package test
30+
allow := true
31+
32+
test_rule {
33+
allow = true
34+
}`,
35+
},
36+
contentAfterFix: `package test
37+
allow := true
38+
39+
test_rule {
40+
allow = true
41+
}`,
42+
fixExpected: false,
43+
runtimeOptions: &RuntimeOptions{},
2944
},
3045
"single change": {
31-
fc: &FixCandidate{Filename: "test.rego", Contents: "package test\n\nallow := true\nallow = true\n"},
32-
contentAfterFix: "package test\n\nallow := true\nallow == true\n",
33-
fixExpected: true,
34-
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 4, Column: 7}}},
46+
fc: &FixCandidate{
47+
Filename: "test.rego",
48+
Contents: `package test
49+
allow := true
50+
51+
test_rule {
52+
allow = true
53+
}`,
54+
},
55+
contentAfterFix: `package test
56+
allow := true
57+
58+
test_rule {
59+
allow == true
60+
}`,
61+
fixExpected: true,
62+
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 5, Column: 2}}},
3563
},
3664
"bad change": {
37-
fc: &FixCandidate{Filename: "test.rego", Contents: "package test\n\nallow := true\nallow = true\n"},
38-
contentAfterFix: "package test\n\nallow := true\nallow = true\n",
39-
fixExpected: false,
40-
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 1, Column: 1}}},
65+
fc: &FixCandidate{
66+
Filename: "test.rego",
67+
Contents: `package test
68+
allow := true
69+
allow = true
70+
71+
test_rule {
72+
allow = false
73+
}`,
74+
},
75+
contentAfterFix: `package test
76+
allow := true
77+
allow = true
78+
79+
test_rule {
80+
allow = false
81+
}`,
82+
fixExpected: false,
83+
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 1, Column: 1}}},
4184
},
4285
"many changes": {
4386
fc: &FixCandidate{
4487
Filename: "test.rego",
45-
Contents: "package test\n\nallow := true\n\nallow = true\nallow = false\n",
88+
Contents: `package test
89+
allow := true
90+
91+
test_rule {
92+
allow = true
93+
allow = false
94+
}`,
4695
},
47-
contentAfterFix: "package test\n\nallow := true\n\nallow == true\nallow == false\n",
48-
fixExpected: true,
49-
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 5, Column: 7}, {Row: 6, Column: 7}}},
96+
contentAfterFix: `package test
97+
allow := true
98+
99+
test_rule {
100+
allow == true
101+
allow == false
102+
}`,
103+
fixExpected: true,
104+
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 5, Column: 2}, {Row: 6, Column: 2}}},
50105
},
51106
"different columns": {
52107
fc: &FixCandidate{
53108
Filename: "test.rego",
54-
Contents: "package test\n\nlong := true\n\nlonger = true\nlongest = false\n",
109+
Contents: `package test
110+
allow := true
111+
112+
test_rule {
113+
allow = true
114+
allow = false
115+
}`,
55116
},
56-
contentAfterFix: "package test\n\nlong := true\n\nlonger == true\nlongest == false\n",
57-
fixExpected: true,
58-
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 5, Column: 8}, {Row: 6, Column: 9}}},
117+
contentAfterFix: `package test
118+
allow := true
119+
120+
test_rule {
121+
allow == true
122+
allow == false
123+
}`,
124+
fixExpected: true,
125+
runtimeOptions: &RuntimeOptions{Locations: []report.Location{{Row: 5, Column: 2}, {Row: 6, Column: 2}}},
59126
},
60127
}
61128
for testName, tc := range testCases {
@@ -77,7 +144,7 @@ func TestPreferEqualsComparison(t *testing.T) {
77144
return
78145
}
79146

80-
if tc.fixExpected && fixResults[0].Contents != tc.contentAfterFix {
147+
if diff := cmp.Diff(fixResults[0].Contents, tc.contentAfterFix); tc.fixExpected && diff != "" {
81148
t.Fatalf(
82149
"unexpected content, got:\n%s---\nexpected:\n%s---",
83150
fixResults[0].Contents,

0 commit comments

Comments
 (0)