From 904a7c53e8a1afec5af8b05bb11f7da846595a09 Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Wed, 28 Sep 2022 06:26:34 -0400 Subject: [PATCH] Fix `getSnippetHash` not considering all files (#765) * Fix `getSnippetHash` not considering all files Made a stupid mistake in the previous PR: https://github.com/grafana/tanka/pull/759 This fixes it and adds another benchmark test to ensure it doesn't happen again. I also removed the Github Actions benchmark test, as it's not really useful, anytime we change the tests, we'll get erroneous results which will be annoying. Instead, I added the benchmark tests to the Drone run, we can compare whenever we want. * linting * Add changelog, will release straight away --- .github/workflows/benchstat-pr.yml | 75 --------------- CHANGELOG.md | 7 ++ Makefile | 2 +- pkg/jsonnet/imports.go | 2 +- pkg/jsonnet/imports_test.go | 141 ++++++++++++++++++----------- 5 files changed, 96 insertions(+), 131 deletions(-) delete mode 100644 .github/workflows/benchstat-pr.yml diff --git a/.github/workflows/benchstat-pr.yml b/.github/workflows/benchstat-pr.yml deleted file mode 100644 index 08871dc78..000000000 --- a/.github/workflows/benchstat-pr.yml +++ /dev/null @@ -1,75 +0,0 @@ -name: benchstat - -on: [pull_request] - -jobs: - benchstat: - runs-on: ubuntu-latest - steps: - - name: Install Go - uses: actions/setup-go@v3 - with: - go-version: '1.19' - - # Generate benchmark report for main branch - - name: Checkout - uses: actions/checkout@v3 - with: - ref: main - - name: Benchmark - run: go test ./... -count=10 -run="^$" -bench=. -benchmem | tee -a bench.txt - - name: Upload Benchmark - uses: actions/upload-artifact@v3 - with: - name: bench-current - path: bench.txt - - # Generate benchmark report for the PR - - name: Checkout - uses: actions/checkout@v3 - - name: Benchmark - run: go test ./... -count=10 -run="^$" -bench=. -benchmem | tee -a bench.txt - - name: Upload Benchmark - uses: actions/upload-artifact@v3 - with: - name: bench-incoming - path: bench.txt - - # Compare the two reports - - name: Checkout - uses: actions/checkout@v3 - - name: Install benchstat - run: go install golang.org/x/perf/cmd/benchstat@latest - - name: Download Incoming - uses: actions/download-artifact@v3 - with: - name: bench-incoming - path: bench-incoming - - name: Download Current - uses: actions/download-artifact@v3 - with: - name: bench-current - path: bench-current - - name: Benchstat Results - run: benchstat bench-current/bench.txt bench-incoming/bench.txt | tee -a benchstat.txt - - name: Upload benchstat results - uses: actions/upload-artifact@v3 - with: - name: benchstat - path: benchstat.txt - - name: Read benchstat.txt - id: benchstat_content - uses: juliangruber/read-file-action@v1 - with: - path: ./benchstat.txt - - name: Post PR Comment - uses: thollander/actions-comment-pull-request@v1 - with: - message: | - Benchstat (compared to main): - - ``` - ${{ steps.benchstat_content.outputs.content }} - ``` - comment_includes: 'Benchstat (compared to main):' - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 59c9ee34c..a93b2464b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## 0.23.1 (2022-09-28) + +### Bug Fixes/Enhancements + +- **export**: Fix `getSnippetHash` not considering all files + **[#765](https://github.com/grafana/tanka/pull/765)** + ## 0.23.0 (2022-09-26) ### Features diff --git a/Makefile b/Makefile index 0d3df8b06..098881121 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ lint: $(GOLINTER) $(GOLINTER) run test: - go test ./... + go test ./... -bench=. -benchmem # Compilation dev: diff --git a/pkg/jsonnet/imports.go b/pkg/jsonnet/imports.go index 1c1a59a5d..cbe2acb2a 100644 --- a/pkg/jsonnet/imports.go +++ b/pkg/jsonnet/imports.go @@ -204,7 +204,7 @@ func findImportRecursiveRegexp(list map[string]bool, vm *jsonnet.VM, filename, c } if list[abs] { - return nil + continue } list[abs] = true diff --git a/pkg/jsonnet/imports_test.go b/pkg/jsonnet/imports_test.go index fe940c91b..7c3b5e736 100644 --- a/pkg/jsonnet/imports_test.go +++ b/pkg/jsonnet/imports_test.go @@ -28,81 +28,114 @@ func TestTransitiveImports(t *testing.T) { }, imports) } -const testFile = ` -local localImport = ; -local myFunc = function() ; +func BenchmarkGetSnippetHash(b *testing.B) { + for _, tc := range []struct { + name string + importFromMain bool + expectedHash string + }{ + { + name: "all-imported-from-main", + importFromMain: true, + expectedHash: "ktY8NYZOoPacsNYrH7-DslRgLG54EMRdk3MQSM3vcUg=", + }, + { + name: "deeply-nested", + importFromMain: false, + expectedHash: "W1Q_uS6jTGcsd7nvJfc-i785sqjBmOzfOAzqzhXVc0A=", + }, + } { + b.Run(tc.name, func(b *testing.B) { + // Create a very large and complex project + tempDir := b.TempDir() + generateTestProject(b, tempDir, 10000, tc.importFromMain) + + // Create a VM. It's important to reuse the same VM + // While there is a caching mechanism that normally shouldn't be shared in a benchmark iteration, + // it's useful to evaluate its impact here, because the caching will also improve the evaluation performance afterwards. + vm := MakeVM(Opts{ImportPaths: []string{tempDir}}) + content, err := os.ReadFile(filepath.Join(tempDir, "main.jsonnet")) + require.NoError(b, err) + + // Run the benchmark + mainPath := filepath.Join(tempDir, "main.jsonnet") + c := string(content) + b.ResetTimer() + for i := 0; i < b.N; i++ { + fileHashes = sync.Map{} + hash, err := getSnippetHash(vm, mainPath, c) + require.NoError(b, err) + require.Equal(b, tc.expectedHash, hash) + } + }) + } +} -{ - local this = self, +func generateTestProject(t testing.TB, dir string, depth int, importAllFromMain bool) []string { + t.Helper() - attribute: { - name: 'test', - value: self.name, - otherValue: 'other ' + self.value, - }, - nested: { + const testFile = ` + local localImport = ; + local myFunc = function() ; + + { + local this = self, + + attribute: { + name: 'test', + value: self.name, + otherValue: 'other ' + self.value, + }, nested: { nested: { nested: { - nested1: { - nested: { - nested1: { - nested: { - attribute: , + nested: { + nested1: { + nested: { + nested1: { + nested: { + attribute: , + }, + }, + nested2: { + strValue: this.nested.nested.nested, }, - }, - nested2: { - strValue: this.nested.nested.nested, }, }, - }, - nested2: { - intValue: 1, - importValue: , + nested2: { + intValue: 1, + importValue: , + }, }, }, }, }, - }, + + other: myFunc(), + useLocal: localImport, + }` - other: myFunc(), - useLocal: localImport, -}` - -func BenchmarkGetSnippetHash(b *testing.B) { - // Create a very large and complex project - tempDir := b.TempDir() + var allFiles []string var mainContentSplit []string for i := 0; i < 1000; i++ { mainContentSplit = append(mainContentSplit, fmt.Sprintf("(import 'file%d.libsonnet')", i)) - } - require.NoError(b, os.WriteFile(filepath.Join(tempDir, "main.jsonnet"), []byte(strings.Join(mainContentSplit, " + ")), 0644)) - for i := 0; i < 1000; i++ { + filePath := filepath.Join(dir, fmt.Sprintf("file%d.libsonnet", i)) err := os.WriteFile( - filepath.Join(tempDir, fmt.Sprintf("file%d.libsonnet", i)), + filePath, []byte(strings.ReplaceAll(testFile, "", fmt.Sprintf("import 'file%d.libsonnet'", i+1))), 0644, ) - require.NoError(b, err) + require.NoError(t, err) + allFiles = append(allFiles, filePath) } - require.NoError(b, os.WriteFile(filepath.Join(tempDir, "file1000.libsonnet"), []byte(`"a string"`), 0644)) - - // Create a VM. It's important to reuse the same VM - // While there is a caching mechanism that normally shouldn't be shared in a benchmark iteration, - // it's useful to evaluate its impact here, because the caching will also improve the evaluation performance afterwards. - vm := MakeVM(Opts{ImportPaths: []string{tempDir}}) - content, err := os.ReadFile(filepath.Join(tempDir, "main.jsonnet")) - require.NoError(b, err) - - // Run the benchmark - mainPath := filepath.Join(tempDir, "main.jsonnet") - c := string(content) - b.ResetTimer() - for i := 0; i < b.N; i++ { - fileHashes = sync.Map{} - hash, err := getSnippetHash(vm, mainPath, c) - require.NoError(b, err) - require.Equal(b, "XrkW8N2EvkFMvdIuHTsGsQespVUl9_xiFmM7v1mqX5s=", hash) + if !importAllFromMain { + mainContentSplit = append(mainContentSplit, "import 'file0.libsonnet'") } + require.NoError(t, os.WriteFile(filepath.Join(dir, "main.jsonnet"), []byte(strings.Join(mainContentSplit, " + ")), 0644)) + allFiles = append(allFiles, filepath.Join(dir, "main.jsonnet")) + require.NoError(t, os.WriteFile(filepath.Join(dir, "file1000.libsonnet"), []byte(`"a string"`), 0644)) + allFiles = append(allFiles, filepath.Join(dir, "file1000.libsonnet")) + + return allFiles }