Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acc: add a helper to diff with replacements #2352

Merged
merged 6 commits into from
Feb 14, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions acceptance/acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const (
EntryPointScript = "script"
CleanupScript = "script.cleanup"
PrepareScript = "script.prepare"
ReplsFile = "repls.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment what's this file used for? From first glance it's not clear if it's an input for replacement or some sort of output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

MaxFileSize = 100_000
)

Expand All @@ -65,6 +66,10 @@ var Scripts = map[string]bool{
PrepareScript: true,
}

var Ignored = map[string]bool{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to make this configurable, maybe in the future?

Copy link
Contributor Author

@denik denik Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can discuss it once we have use case for it.

ReplsFile: true,
}

func TestAccept(t *testing.T) {
testAccept(t, InprocessMode, SingleTest)
}
Expand Down Expand Up @@ -152,6 +157,8 @@ func testAccept(t *testing.T, InprocessMode bool, singleTest string) int {
testdiff.PrepareReplacementSdkVersion(t, &repls)
testdiff.PrepareReplacementsGoVersion(t, &repls)

repls.SetPath(cwd, "[TESTROOT]")

repls.Repls = append(repls.Repls, testdiff.Replacement{Old: regexp.MustCompile("dbapi[0-9a-f]+"), New: "[DATABRICKS_TOKEN]"})

testDirs := getTests(t)
Expand Down Expand Up @@ -310,6 +317,12 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
// User replacements come last:
repls.Repls = append(repls.Repls, config.Repls...)

if config.SaveRepls {
replsJson, err := json.MarshalIndent(repls.Repls, "", " ")
require.NoError(t, err)
testutil.WriteFile(t, filepath.Join(tmpDir, ReplsFile), string(replsJson))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be unlikely to happen but if my test script also produces repls.json this will override it, correct? Shall we error out in this case just to be safe?

Copy link
Contributor Author

@denik denik Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is written before your script is run, so your script will override it, which will break diff.py if you use it.

You should be able to modify your script to use different file or rename the files like we do with .gitignore.

}

if coverDir != "" {
// Creating individual coverage directory for each test, because writing to the same one
// results in sporadic failures like this one (only if tests are running in parallel):
Expand All @@ -320,6 +333,10 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
cmd.Env = append(cmd.Env, "GOCOVERDIR="+coverDir)
}

absDir, err := filepath.Abs(dir)
require.NoError(t, err)
cmd.Env = append(cmd.Env, "TESTDIR="+absDir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't absDir value be in quotes? It might contain spaces at least on Windows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes on Windows CI and it's also not the only directory we have, so judging from practice, no. Unless you have a Windows machine where it does not work without quotes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it passes on Windows CI it's fine I guess


// Write combined output to a file
out, err := os.Create(filepath.Join(tmpDir, "output.txt"))
require.NoError(t, err)
Expand Down Expand Up @@ -368,6 +385,9 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
if _, ok := outputs[relPath]; ok {
continue
}
if _, ok := Ignored[relPath]; ok {
continue
}
unexpected = append(unexpected, relPath)
if strings.HasPrefix(relPath, "out") {
// We have a new file starting with "out"
Expand Down
56 changes: 56 additions & 0 deletions acceptance/bin/diff.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#!/usr/bin/env python3
"""This script implements "diff -r -U2 dir1 dir2" but applies replacements first"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have a chance to measure performance difference between running this test with diff and with repls.json writing + diff.py?

Copy link
Contributor Author

@denik denik Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand -- diff is doing the wrong thing there, e.g. it will look at actual username rather than [USERNAME] and detect that as a difference. So performance is not important because they are not equivalent, functionality-wise.

We can bench diff.py against diff but even if it's 2x slower there is no action to take, because we use diff.py for handling replacements.

That's said I don't think diffing will be a bottleneck in your tests, so it makes sense to prefer diff.py because it has less cross-platform quirks than diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relates to discussion in the other thread about potentially replacing the output without storing repl.json file and using diff instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since it's not really possible to use diff anyway, performance here indeed doesn't really matter


import sys
import difflib
import json
import re
from pathlib import Path


def replaceAll(patterns, s):
for comp, new in patterns:
s = comp.sub(new, s)
return s


def main():
d1, d2 = sys.argv[1:]
d1, d2 = Path(d1), Path(d2)

with open("repls.json") as f: # Must have 'SaveRepls = true' in test.toml
repls = json.load(f)

patterns = []
for r in repls:
try:
c = re.compile(r["Old"])
patterns.append((c, r["New"]))
except re.error as e:
print(f"Regex error for pattern {r}: {e}", file=sys.stderr)

files1 = [str(p.relative_to(d1)) for p in d1.rglob("*") if p.is_file()]
files2 = [str(p.relative_to(d2)) for p in d2.rglob("*") if p.is_file()]

set1 = set(files1)
set2 = set(files2)

for f in sorted(set1 | set2):
p1 = d1 / f
p2 = d2 / f
if f not in set2:
print(f"Only in {d1}: {f}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it a bit more explicit phrasing? Something like "File X is found only in Y directory". When I was reading test output it was not immediately clear what's going on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follows "diff -r" phrasing. If you're familiar with that, it makes sense.

elif f not in set1:
print(f"Only in {d2}: {f}")
else:
a = [replaceAll(patterns, x) for x in p1.read_text().splitlines(True)]
b = [replaceAll(patterns, x) for x in p2.read_text().splitlines(True)]
if a != b:
p1_str = p1.as_posix()
p2_str = p2.as_posix()
for line in difflib.unified_diff(a, b, p1_str, p2_str, "", "", 2):
print(line, end="")


if __name__ == "__main__":
main()
3 changes: 3 additions & 0 deletions acceptance/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ type TestConfig struct {
// If true, do not run this test against cloud environment
LocalOnly bool

// if true, save file repls.json with all the replacemnts
SaveRepls bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead of saving and then loading replacements just do the output replacement always (maybe to a separate temp folder) and then compare it? Or just always save it and ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid adding this overhead to all tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, File i/o is cheap so this should not add much overhead? Doing replacement on all output files by default makes sense since we do that anyways when performing comparisons. WDYT?

Copy link
Contributor

@shreyas-goenka shreyas-goenka Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also get to avoid the added complexity of having SaveRepls and repls.json.

And we could use diff directly instead of relying on the python script to perform replacements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denik by overhead you mean performance overhead? If so, how do we balance between performance and code complexity then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I benchmarked it, there is no detectable difference on my laptop, so I removed the option: c8b4196

We have open PRs with 100s of tests #2260
so we might reconsider it then if we measure that it makes a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we could use diff directly instead of relying on the python script to perform replacements.

I don't see how that is possible, the replacements are applied by test runner but on file system the files are without replacements so diff will see the actual values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the files are without replacements so diff will see the actual values

I was proposing we change this. If the files on the file system are with the replacement then we could use diff directly.

But on thinking about this more it would make debugging harder since since you don't know the pre-replacement values. The python approach makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shreyas-goenka I don't think this is possible as the replacements could be done only after script is finished and diff call is a part of the script

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could introduce some sort of post script run validation script which would call the diff and compare expected output but it might be more complex


// List of additional replacements to apply on this test.
// Old is a regexp, New is a replacement expression.
Repls []testdiff.Replacement
Expand Down
7 changes: 7 additions & 0 deletions acceptance/selftest/diff/out_dir_a/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Hello!
{
"id": "[USERID]",
"userName": "[USERNAME]"
}

Footer
7 changes: 7 additions & 0 deletions acceptance/selftest/diff/out_dir_b/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Hello!
{
"id": "[UUID]",
"userName": "[USERNAME]"
}

Footer
13 changes: 13 additions & 0 deletions acceptance/selftest/diff/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

>>> diff.py out_dir_a out_dir_b
Only in out_dir_a: only_in_a
Only in out_dir_b: only_in_b
--- out_dir_a/output.txt
+++ out_dir_b/output.txt
@@ -1,5 +1,5 @@
Hello!
{
- "id": "[USERID]",
+ "id": "[UUID]",
"userName": "[USERNAME]"
}
17 changes: 17 additions & 0 deletions acceptance/selftest/diff/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
mkdir out_dir_a
mkdir out_dir_b

touch out_dir_a/only_in_a
touch out_dir_b/only_in_b

echo Hello! >> out_dir_a/output.txt
echo Hello! >> out_dir_b/output.txt

curl -s $DATABRICKS_HOST/api/2.0/preview/scim/v2/Me >> out_dir_a/output.txt
printf "\n\nFooter" >> out_dir_a/output.txt
printf '{\n "id": "7d639bad-ac6d-4e6f-abd7-9522a86b0239",\n "userName": "[USERNAME]"\n}\n\nFooter' >> out_dir_b/output.txt

# Unlike regular diff, diff.py will apply replacements first before doing the comparison
errcode trace diff.py out_dir_a out_dir_b

rm out_dir_a/only_in_a out_dir_b/only_in_b
1 change: 1 addition & 0 deletions acceptance/selftest/diff/test.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SaveRepls = true
1 change: 1 addition & 0 deletions acceptance/selftest/test.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
LocalOnly = true