acc: add a helper to diff with replacements #2352

denik · 2025-02-13T09:19:44Z

Changes

diff.py is like "diff -r -U2" but it applies replacements first to the argument.

This allows comparing different output files and directories but ignore differences that are going to be replaced by placeholders.

This is useful for tests that record large amount of files, specifically "bundle init" with standard templates. In those tests, changing one parameter results in a small diff so recording the full directory is not helpful, because it's hard to see what changed there. I'm using it in implementation of serverless mode for templates that need it: #2348 The serverless templates are slightly different from classic, capturing the diff helps to see exactly where.

Related small changes:

Add [TESTROOT] replacement for absolute path to acceptance directory in git repo.
Add $TESTDIR env var for absolute path to a given test in git repo.

Tests

New test acceptance/selftest/diff to test the helper.
Via Support serverless mode in default-python template #2348 which makes use of this feature.

diff.py is like "diff -r -U2" but it applies replacements first to the argument. This allows comparing different output files and directories but ignore differences that are going to be replaced by placeholders. This is useful for tests that record large amount of files, specifically "bundle init" with standard templates. In those tests, changing one parameter results in a small diff so recording the full directory is not helpful, because it's hard to see what changed there. I'm using it in implementation of serverless mode for templates that need it: #2348 Related small changes: add [TESTROOT] replacement for absolute path to acceptance directory in git repo. Add $TESTDIR env var for absolute path to a given test in git repo.

andrewnester · 2025-02-14T09:14:26Z

acceptance/acceptance_test.go

@@ -56,6 +56,7 @@ const (
 	EntryPointScript = "script"
 	CleanupScript    = "script.cleanup"
 	PrepareScript    = "script.prepare"
+	ReplsFile        = "repls.json"


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

Added a comment.

andrewnester · 2025-02-14T09:15:09Z

acceptance/acceptance_test.go

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

+var Ignored = map[string]bool{


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

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

andrewnester · 2025-02-14T09:19:05Z

acceptance/acceptance_test.go

@@ -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)


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

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?

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

andrewnester · 2025-02-14T09:25:38Z

acceptance/bin/diff.py

+        p1 = d1 / f
+        p2 = d2 / f
+        if f not in set2:
+            print(f"Only in {d1}: {f}")


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

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

andrewnester · 2025-02-14T09:30:27Z

acceptance/config_test.go

@@ -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


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?

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

+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?

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.

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

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.

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.

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.

@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

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

andrewnester · 2025-02-14T10:30:58Z

acceptance/acceptance_test.go

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


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?

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.

andrewnester · 2025-02-14T10:35:22Z

acceptance/bin/diff.py

@@ -0,0 +1,56 @@
+#!/usr/bin/env python3
+"""This script implements "diff -r -U2 dir1 dir2" but applies replacements first"""


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

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.

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

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

denik added 2 commits February 13, 2025 10:11

pathlib

9651bcb

denik requested review from pietern, andrewnester and shreyas-goenka as code owners February 13, 2025 09:19

denik temporarily deployed to test-trigger-is February 13, 2025 09:19 — with GitHub Actions Inactive

ruff

bca4e6c

denik temporarily deployed to test-trigger-is February 13, 2025 09:27 — with GitHub Actions Inactive

Disable selftest on cloud

8db21ba

denik temporarily deployed to test-trigger-is February 13, 2025 10:03 — with GitHub Actions Inactive

denik mentioned this pull request Feb 13, 2025

Support serverless mode in default-python template #2348

Open

andrewnester reviewed Feb 14, 2025

View reviewed changes

add a comment

b04d805

denik temporarily deployed to test-trigger-is February 14, 2025 09:56 — with GitHub Actions Inactive

denik requested a review from andrewnester February 14, 2025 09:57

get rid of SaveRepls config setting

c8b4196

denik temporarily deployed to test-trigger-is February 14, 2025 10:29 — with GitHub Actions Inactive

andrewnester reviewed Feb 14, 2025

View reviewed changes

denik requested a review from andrewnester February 14, 2025 10:33

andrewnester reviewed Feb 14, 2025

View reviewed changes

denik requested a review from andrewnester February 14, 2025 10:35

andrewnester approved these changes Feb 14, 2025

View reviewed changes

denik added this pull request to the merge queue Feb 14, 2025

Merged via the queue into main with commit c0a56a9 Feb 14, 2025
9 checks passed

denik deleted the denik/acc-diff branch February 14, 2025 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acc: add a helper to diff with replacements #2352

acc: add a helper to diff with replacements #2352

denik commented Feb 13, 2025 •

edited

Loading

andrewnester Feb 14, 2025

denik Feb 14, 2025

andrewnester Feb 14, 2025

denik Feb 14, 2025 •

edited

Loading

andrewnester Feb 14, 2025

denik Feb 14, 2025

andrewnester Feb 14, 2025

andrewnester Feb 14, 2025

denik Feb 14, 2025

andrewnester Feb 14, 2025

denik Feb 14, 2025

shreyas-goenka Feb 14, 2025

shreyas-goenka Feb 14, 2025 •

edited

Loading

andrewnester Feb 14, 2025

denik Feb 14, 2025

denik Feb 14, 2025

shreyas-goenka Feb 14, 2025

andrewnester Feb 14, 2025

andrewnester Feb 14, 2025

andrewnester Feb 14, 2025

denik Feb 14, 2025 •

edited

Loading

andrewnester Feb 14, 2025

denik Feb 14, 2025 •

edited

Loading

andrewnester Feb 14, 2025

andrewnester Feb 14, 2025

		@@ -0,0 +1,56 @@
		#!/usr/bin/env python3
		"""This script implements "diff -r -U2 dir1 dir2" but applies replacements first"""

acc: add a helper to diff with replacements #2352

acc: add a helper to diff with replacements #2352

Conversation

denik commented Feb 13, 2025 • edited Loading

Changes

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denik Feb 14, 2025 • edited Loading

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

shreyas-goenka Feb 14, 2025 • edited Loading

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

denik Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denik Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denik commented Feb 13, 2025 •

edited

Loading

denik Feb 14, 2025 •

edited

Loading

shreyas-goenka Feb 14, 2025 •

edited

Loading

denik Feb 14, 2025 •

edited

Loading

denik Feb 14, 2025 •

edited

Loading